simonsobs / simonsobs/socs

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

Open
#877 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

agent: hwp supervisor enhancement
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.