HWP Supervisor: Suggestion to remove link between sign of frequency and direction in `pid_to_freq`

[BrianJKoopman]

This came up in discussion on sorunlib (see simonsobs/sorunlib#179 (review)) -- but I think we should modify the HWP Supervisor agent's pid_to_freq method to remove the inherent relationship between the sign of the target_freq argument and the direction the HWP spins in.

As currently written a client using pid_to_freq needs to know the relationship between the direction ('cw' or 'ccw') and the sign of the target_freq, i.e. that +2.0 Hz corresponds to 'ccw'. But this correspondence can change depending on the hardware/software configuration, i.e. if --forward-dir changes in the SCF.

I think it would be better to change pid_to_freq to take an unsigned target_freq and a direction argument (either 'cw' or 'ccw'). The agent can then determine the appropriate direction to rotate in via something like:

if params['direction'] == 'cw':
    d = '0' if self.forward_is_cw else '1'
elif params['direction'] == 'ccw':
    d = '1' if self.forward_is_cw else '0'

The motivation here is to remove the hardcoded correspondence that is currently needed in downstream code, i.e. in sorunlib:

if target_hwp_direction == 'ccw':
    run.hwp.set_freq(freq=2.0)
elif target_hwp_direction == 'cw':
    run.hwp.set_freq(freq=-2.0)

It would also make schedules perhaps more readable, since the unfamiliar user would read something like:

run.hwp.set_freq(freq=2, direction='cw')

instead of just:

run.hwp.set_freq(freq=-2)

@ykyohei I'd be curious to hear your thoughts.

[ykyohei]

I see the point that code will be better, are you suggesting breaking change?
If the motivation is to reduce hard coding outside HWPSupervisor, is it fine to keep accepting signed frequency argument but also accept direction argument? For example, if the direction is specified, we will ignore the sign of the frequency or raise an error with a negative frequency.

if we make breaking change, we have to change ocs-web and scheduler etc too. +/- is used in master schedule.
https://github.com/simonsobs/SAT-scan-schedules/blob/main/20250411_d-40%2C-10_e60_s0.5%2C0.8_a49_j2025-02-15T12%3A00%2B00%3A00_n365.txt#L22

I think suggested change will not make system robust againt hardware/software config change.
Correspondence between 2Hz and ccw is determined by SO coords convention so this should not change.
hardware config will not change because this is determined by the polarity of cryogenic instruments.
If accidentally change software config, I think this leads to wrong tag of observation even after the suggested change.

[BrianJKoopman]

We can make the change in a non-breaking manner, either as you suggest by making the direction argument optional (and ignoring the sign on freq if provided), or by making it a separate task altogether.

And right, this won't make us totally robust against hardware/software config changes. What it does do is limit the impact on downstream code, namely sorunlib. After needing to think through the convention it occurred to me that if we had originally implemented it in the manner I'm suggesting, sorunlib wouldn't need to know anything about the convention/implementation details, and so wouldn't have been impacted by the bug and misconfiguration we had.

All that said, this probably isn't very critical, as we've thought through things and have convinced ourselves the convention is correct as is. I just wanted to get this down on paper while I was thinking about it.

As for that schedule you listed, I'm not familiar enough with the code that generates that to know whether it really presents a problem there too.