HWP Supervisor: Suggestion to remove link between sign of frequency and direction in `pid_to_freq`
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 17
- Forks
- 20
- Avg merge
- 1h 22m
- Merged PRs (30d)
- 2
Description
This came up in discussion on sorunlib (see https://github.com/simonsobs/sorunlib/pull/179#pullrequestreview-2837972863) -- 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.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with socs/agents/hwp_supervisor/agent.py at the pid_to_freq method (lines 1688-1732), then read the linked sorunlib discussion for the motivation and compatibility concerns. Trace how target_freq and direction are passed by clients, including the shown sorunlib set_freq calls. Done means the API accepts an unsigned frequency and explicit cw/ccw direction without relying on the sign-to-direction convention, with affected callers remaining understandable.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100