Add wiregrid.time_constant function

[d-hoshino2626]

I added some APIs for a time constant measurement with the wire grid and the HWP into the wiregrid.py.
Here is the added functions and its explanation:

1. _check_wiregrid_position()
   • This function checks the wire grid is off the window or on the window from the logs for the limit switches.
2. _stop_hwp_with_wiregrid()
   • This function confirms the wire grid is on the window and stops the HWP.
3. _spin_hwp_with_wiregrid(target_hwp_direction)
   • This function confirms the wire grid is on the window and spins up the HWP in the direction same as target_hwp_direction.
   • target_hwp_direction is bool argument, 'forward' or 'backward'.
4. _reverse_hwp_with_wiregrid(initial_hwp_direction, streaming=False, stepwise_before=False, stepwise_after=False)
   • This function reverses the HWP spinning direction from initial_hwp_direction using _stop_hwp_with_wiregrid() and _spin_hwp_with_wiregrid(target_hwp_direction)
   • Also, the stepwise rotation can be performed before and/or after the reversing. The bool arguments stepwise_before and stepwise_after are to identify whether performing them or not.
   • SMuRF streaming can be performed during the stepwise rotation and HWP speed change.
5. time_constant(initial_hwp_direction, stepwise_first=True, stepwise_last=True, stepwise_mid=False, repeat=1)
   • This is the main public API. This function inserts the wire grid, changes the HWP direction as many times as repeat, and ejects the wire grid.
   • Also, it takes bias steps before and after the insertion and ejection, execute the stepwise rotations before, in the middle of, or after the HWP direction changes.

Any questions and comments are welcome.

[ykyohei]

I comment in terms of hwp controls for this type of measurements that spins up and down hwp.

1. I suggest streaming separately for forward spin and backward spin. This is because hwp solution pipeline assumes constant rotation direction within the data range. It requires more work to analyze it if both forward and reverse spin are included in the data range.

2. For the hwp rotation direction estimation, you can just use. apply_hwp_angle_model(tod, on_sign_ambiguous='offcenter'). It might be possible to simplify functions. The hwp rotation direction estimation has to be different from cmb observations bacause hwp_solution.pid_direction is not reliable for spin down measurement. This is because commanded spin direction and actual spin direction is different when hwp is spinning down. hwp_solution.offcenter_direction will be the robust rotation direction estimator for this hwp spin down up measurements.

[d-hoshino2626]

Thanks Kyohei!

1. I consulted with Hironobu about whether it is better to separate the stream or not and his opinion was not to separate. I heard from him that the HWP pipeline works well if the stream is separated properly by analysis and it is not difficult. He will mainly work on this analysis, so it would be better to follow his opinion, I think.
2. Thanks for your information! I will re-write it using apply_hwp_angle_model(tod, on_sign_ambiguous='offcenter').

[sadachi5]

I add comments around insert() and eject().

[ykyohei]

1. I consulted with Hironobu about whether it is better to separate the stream or not and his opinion was not to separate. I heard from him that the HWP pipeline works well if the stream is separated properly by analysis and it is not difficult. He will mainly work on this analysis, so it would be better to follow his opinion, I think.

I'm concerned about future automation of gl calibration. I will let you decide, but I don't see any disadvantage of splitting stream. If you split, you don't need to re-calculate hwp solution.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 4.76190% with 100 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@1fc4725). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/sorunlib/wiregrid.py	4.76%	100 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #179   +/-   ##
=======================================
  Coverage        ?   84.15%           
=======================================
  Files           ?       10           
  Lines           ?      631           
  Branches        ?        0           
=======================================
  Hits            ?      531           
  Misses          ?      100           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hnakata-JP]

Thanks, Kyohei and Daiki.
As Daiki says, I prefer taking the stream all at once. In my previous analysis, I divided the stream by two by the quad key of hwp_solution and removed about 2000 samples just before/after the split point. What I am concerned about is an opportunity for an interruption between these measurements. I think it would be better to remove any room between them in order not to make any other ambiguous gap.

[ykyohei]

Thanks, Kyohei and Daiki. As Daiki says, I prefer taking the stream all at once. In my previous analysis, I divided the stream by two by the quad key of hwp_solution and removed about 2000 samples just before/after the split point. What I am concerned about is an opportunity for an interruption between these measurements. I think it would be better to remove any room between them in order not to make any other ambiguous gap.

OK, I'm fine if you don't mind recalculating the hwp angle.

[BrianJKoopman]

Here's a first pass at a review from me. A couple general comments/comments:

• Please wrap lines to ~80 characters. I'm not super strict on the exact length, but there are some pretty long lines (100+) characters here. Try to be consistent with the existing code here.
• How often will this be run? Daily? Monthly? Once? I was a bit surprised to see the PR, thinking we had implemented the regular calibration in wiregrid.calibrate() already.
• We need tests. I've been pretty meticulous to keep 100% coverage from the get-go in this repo. sorunlib is tricky because I often can't just interrupt daily operations to test some new functionality. Check out the tests/ directory to see existing tests. New tests for this will go in tests/test_wiregrid.py. There are instructions for running tests in the main README. Also, please merge in the latest main, there will be a new code coverage check that runs.

[d-hoshino2626]

Thanks Brian! Sorry for the late reply and the lack of explanation. I'm replying about some discussion points.

• This measurement is to calibrate the detectors' time constant by changing the HWP spin direction with the wiregrid inserted. This will be performed about once a week. Now we change the HWP spin direction once a week, so we will do it on that time.

[d-hoshino2626]

Hi Brian, I editted the time_constant function and some internal functions. I'm sorry for pending so ling time. I'll put the change list here:

1. In _change_wiregird_position(), I changed to use position of session.data in the Wiregrid Actuator Agent.
2. I added _check_hwp_direction() function. This function read the pid_direction of session.data in the HWP PID Agent via the HWP Supervisor Agent and return the current HWP rotation direction, forward or backward. 0 of pid_direction is forward and 1 is backward.
3. I deleted _stop_hwp_with_wiregrid() and _spin_hwp_with_wiregrid(). These are integrated _reverse_hwp_direction() function.
4. _reverse_hwp_directoin() function reverse the HWP in the opposite direction of initial_hwp_direction argument. Before and after, the stepwise rotation of wiregrid is executed as necessary. Also, I moved smurf stream parts from here to time_constant() function.
5. In time_constant() function, I deleted some argments, initial_hwp_direction, stepwise_first, stepwise_last, stepwise_mid. The reasons of deleting are here.
   1. initial_hwp_direction: Because _check_hwp_direction() funcition can check the current hwp rotation direction automatically.
   2. stepwise_first, stepwise_last, and stepwise_mid: Basically, we need to do stepwise rotation before/after the time constant measurement in order to check the data quality. I concluded that skipping stepwise rotations is almost never necessary and I removed these arguments.

Also, I added some tests in tests/test_wiregrid.py. The coverage is 100%.

By the way, I'm leaving the SO collaboration by this month and @sadachi5 is going to take over this PR.

[sadachi5]

@BrianJKoopman
Could you check the modification by @d-hoshino2626 ?
We want to merge this new feature for the calibration at SATp1.

[BrianJKoopman]

@BrianJKoopman
Could you check the modification by @d-hoshino2626 ?
We want to merge this new feature for the calibration at SATp1.

Thanks for the ping, this is on my todo list for tomorrow.

[BrianJKoopman]

Thanks @d-hoshino2626 for the changes, especially for implementing the tests! (EDIT: Also, good luck with whatever is next for you after SO!)

@sadachi5, this is looking good, just a couple of small things.

[sadachi5]

@BrianJKoopman
I largely modified this script on two points:

1. Split the stream between stopping and spinning up the hwp
   Add wiregrid.time_constant function #179 (comment)
2. Change labeling of hwp rotation direction from forward/backward to cw/ccw.
   Add wiregrid.time_constant function #179 (comment)

The remaining issue is about the bias_step arguments. (I hope this issue will be resolved soon.)
#179 (comment)

I think this script is almost ready for implementation.
Please take another look at it.

[ykyohei]

This looks good, thank you for the changes.
Sorry my comment on bias_step was wrong. it's consistent among SATs. bias_dets is different but it's not used here, so it should be ok.

[BrianJKoopman]

@BrianJKoopman I largely modified this script on two points:

1. Split the stream between stopping and spinning up the hwp
   [add time_constant function #179 (comment)](https://github.com/simonsobs/sorunlib/pull/179#issuecomment-2422445005)

2. Change labeling of hwp rotation direction from `forward`/`backward` to `cw`/`ccw`.
   [add time_constant function #179 (comment)](https://github.com/simonsobs/sorunlib/pull/179#discussion_r2058890318)

The remaining issue is about the bias_step arguments. (I hope this issue will be resolved soon.) #179 (comment)

I think this script is almost ready for implementation. Please take another look at it.

Just taking a quick look I like the change from using 'forward'/'backward' everywhere to using 'cw'/'ccw'. However, I would really like to see simonsobs/socs#863 settle and get merged before giving my final review here, as that has direct impact on the relationship between +/- frequency and 'cw'/'ccw' direction.

[sadachi5]

Rename: hwp.get_direction() --> hwp._get_direction()

[sadachi5]

Hi @BrianJKoopman ,
The PR for HWP seems to be done. Could you resume the review for this PR?

[BrianJKoopman]

Alright, I think this is as ready as it will be. I'm still not a fan of this hardcoded correspondance between direction and the sign of the frequency:

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

This won't improve until the agent is modified though, so I added a note in the comments here for future reference. I think it would be worth removing this, though I expect in the near term the likelihood that this correspondence changes will be low.

Thanks for all the hard work, tracking of different HWP changes, and updates on this one! @sadachi5 @d-hoshino2626 @ykyohei