Acu add sin el nod by mjrand · Pull Request #1072 · simonsobs/socs

mjrand · 2026-06-09T21:28:41Z

Added the gen_sin_el_nod scan generator function into the ACU drivers.

Description

Added a new generator function for generating sinusoidal elevation nods into the ACU agent. This generator will allow us to generate sine wave el nods that do not move in azimuth. This is basically a replica of type3 scan behavior with no azimuth movement.

Motivation and Context

LAT ISO review requested sinusoidal elevation nods with no azimuth movement.

How Has This Been Tested?

I've simmed the gen_sin_el_nod driver function output with success. The generator function should work as intended.

I have not yet tested the changes to the agent generate_scan function or tested an movement on the LAT. This needs to be tested before PR approval.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

mhasself · 2026-06-10T09:48:53Z

Thanks Michael. I recommend that you move this to "draft" PR until we finalize design and test it on a real system.

Initial reactions:

Why should this be a new "scan type" for generate_scan rather than a new, stand-alone process / task? The arguments for this functionality are a tiny subset of the full az scan functionality. It's not like someone would decide, one day, to do an el-nod everywhere that they previously were doing a scan!
In modern application, num_batches and batch_size are deprecated. Let's not introduce new usages ... i.e., they can be removed from your driver function (and just hard-code the current default at the top of the function).
Is there a control on step_time relative to the el_freq? Make sure step_time gives you "enough" points. Fourier limit is 4 but I would think we'd want more like 16.
The generator sets group_flag=0, which means a "stop" can cause the track to terminate at ~any point. That might "work" but it would be best to tune group_flag such that stops will always occur at the bottom of the dip (which is also the starting point).
To support the preceding point, it might be helpful to tweak step_time such that it divides the oscillation period evenly. Then just generate 1 cycle at a time and yield it (another reason to drop the "batch_size" notion). Can discuss more elsewhere. But basically set new_step_time = (el_freq*round((el_freq*step_time)**-1))**-1.

mjrand · 2026-06-10T21:18:34Z

Aaaah I didn't even know there was a draft PR feature... that's super helpful! (I am not great at git... lol). Changed.

There's really no reason other than I wanted to reuse the generate_scan architecture and make it really easy to call from with sorunlib. It should be somewhat easy to make its own process/task. I'll write that in. I wasn't super attached to keeping it in generate_scan instead of making its own process.
Yeah the notes on batches makes sense to me. I can rewrite this to always yield out whole number nods. That should also prevent us from stopping the elevation on a non-zero velocity.... which opens up a question of if we need to also write a smooth-stop generator for elevation (I think we probably do), but that's outside the scope of this PR.
I think step_size = 0.1 is probably good enough for all nods that aren't hilariously high frequency. I think there is a check for this in plan_scan... but it should probably just be in this function calculated like you say.
Agreed we should probably yield whole number nods.
Agreed.

I think rewriting the driver to yield whole number of nods and then rewriting the agent process will be good. One of the worries I had with the current implementation into generate_scan was how plan_scan would interact with az_vel=0. I think it would work... but I think if we make a generate_el_nod function or whatever it could just start at the current az instead of having to run-up to az_vel=0. I'll try this implementation and see how it would work.

mjrand · 2026-06-10T23:54:23Z

Alright I reverted generate_scan and made generate_sin_el_nod. I started it as a copy of generate_scan and then tried to prune back as many things weren't used or weren't necessary. This meant pruning plan_scan and some other misc variables like wait_to_start (which I couldn't find used in generate_scan either...). I also fixed up the generate_sin_el_nod drivers function to only yield full el nods and to always divide them into 16 points (which can be changed...). This means step_time is determined in the driver and doesn't need to be callable in the agent.

mhasself

Thanks! More notes ...

mjrand · 2026-07-03T22:22:13Z

I had to change part of the _run_track function to get el nods to work properly. The previous iteration was only and explicitly checking that the azimuth axis was in ProgramTrack mode. This meant that these el nods that only put the el axis into track_axes was failing because the az axis was in in stop mode. Now _run_track should check that every axis in track_axes (which are the axes we expect to be in ProgramTrack mode) are in ProgramTrack mode. This should benefit type2 and 3 scans too which should be checking that the el axis didn't fail but currently don't.

I also added some comments into the abort checks to make it clear what we're checking for abort mode.

mjrand · 2026-07-03T22:26:14Z

I've tested this current commit on satp5 and generated a few nods with varying num_nods and it worked as expected. Here is num_nods=10 el_freq=0.1 , el_depth=-3 starting at el=60.

The free upload positions plot shows that we're uploading full nods at once too:

mhasself

Thanks -- those plots are quite convincing! Looking good but a few comments.

mhasself · 2026-07-06T23:57:08Z

+            wait_to_start (float): number of seconds to wait before
+                starting a scan, in the case that start_time is None.
+                The default is to compute a minimum time based on the
+                scan parameters and the ACU ramp-up algorithm; this is
+                typically 5-10 seconds.


not a declared arg

I've removed it because I can't seem to see if its used for anything beyond the initial plan and move to scan start (which isn't used in this el nod function).

mhasself

Last batch of comments (?). Mostly cosmetic but not all!

mhasself · 2026-07-16T19:15:24Z

+    def check_completed_nods():
+        return num_nods is None or t * el_freq < num_nods


This is vulnerable to rounding error. Instead of using t, just count the nods and check that.

Changed! Now each loop of generating a nod counts with a num_completed_nod variable

mhasself · 2026-07-16T19:20:12Z

+          Note that all parameters are optional except for
+          el_depth and el_freq.  If only those two parameters
+          are passed, the Process will nod with that depth and frequency,
+          with the azimuth axis held in Stop, indefinitely (until
+          Process .stop method is called)..


In fact only el_depth is mandatory. The indefinitely bit is accurately communicated through the arg num_nods. The write-up here makes it sound like "will nod ... held in Stop" is only the case if one only passes those two parameters. Which is not the case. I think you should just mention here that the az axis is held in Stop.

Fixed up wording to be a bit more clear w.r.t. the az axis. Removed reference to el_freq being mandatory.

mhasself · 2026-07-16T19:21:52Z

+        if el_depth == 0:
+            raise ValueError("El depth must not be equal to 0 for el nod!")


Why not? If zero is not ok ... is 0.0001 ok? Replace with a minimum depth.

Replaced. I chose a super arbitrary value of abs(el_depth) < 0.1 as the condition. I'm not really sure what the real physical minimum should be. Maybe values less than 0.1 work or maybe values a bit higher than 0.1 don't work? I'm not sure without testing. Maybe we need to check this.

mhasself · 2026-07-16T19:22:04Z

+        if el_depth == 0:
+            raise ValueError("El depth must not be equal to 0 for el nod!")
+
+        # Could probably use a condition for if the el endpoints are too close?


Yes, see above...

Removed comment as it is resolved by changing the condition

mhasself · 2026-07-16T19:24:13Z

+                constant-velocity parts of the motion.  The default is
+                None, which will cause an appropriate value to be
+                chosen automatically (typically 0.1 to 1.0).


Default is not None.

Fixed. Default is properly commented as 0.1 seconds

mhasself · 2026-07-16T19:24:58Z

+        scan_params = {k: params.get(k) for k in [
+            'num_nods', 'start_time', 'step_time']
+            if params.get(k) is not None}
+        step_time = scan_params['step_time']


Careful here -- if indeed params['step_time'] could be None, then this bit will fail with KeyError.

changed to step_time = 0.1 if 'step_time' not in scan_params.keys() else scan_params['step_time']

mhasself · 2026-07-16T19:27:31Z

+                    for ax in track_axes:
+                        # Convert between track axes names and status field names.
+                        track_axes_names = {'az': 'Az', 'el': 'El'}  # There's probably a better way to convert these.
+                        if current_modes[track_axes_names[ax]] == 'ProgramTrack':


You could just use current_modes[ax.capitalize()].

mhasself · 2026-07-16T19:34:14Z

+                        start_time=None,
+                        wait_to_start=10.,
+                        step_time=0.05,
+                        batch_size=500):


drop batch_size arg

mhasself · 2026-07-16T19:34:53Z


+def generate_sin_el_nod(az, el_endpoint1, el_endpoint2,
+                        el_freq=.15,
+                        num_batches=None,


drop num_batches arg

mjrand · 2026-08-18T00:03:23Z

Sorry I fell behind on this for a month due to some personal complications. I've resolved many of the lingering comments. One comment in particular about the minimum el_depth may need some further thinking. I set the minimum el_depth to 0.1 degrees rather arbitrarily. We may need to test what the minimum el_depth really is on the platforms (or just pick a value we know no one will ever try to nod smaller than...)

mhasself

Sorry for not getting this sooner. Looks great. Two minor comments. I tested this on the simulator and it is working well.

mhasself · 2026-10-07T13:14:55Z

+    arbitrarily long sin el nods.
+
+    Parameters:
+        el_endpoint1 (float): elevation endpoint for the scan start


Missing "az" in the param list.

mhasself · 2026-10-07T13:15:47Z

+        el_endpoint2 (float): second elevation endpoint of the scan. For
+            constant az scans, this must be equal to el_endpoint1.


Remove "for constant az scans..." part

el_endpoint2 description is fixed.

… moves

mhasself

Thanks -- last few typos (I'll fix those) and we're good to go.

Co-authored-by: Matthew Hasselfield <mhasself@users.noreply.github.com>

mhasself · 2026-10-14T13:00:33Z

@BrianJKoopman This is ready for final review if you want to take a look. I can do the merge (to clean up the commit message).

BrianJKoopman

Took a very quick look. A few docstring/default inconsistencies to fix, otherwise if @mhasself is happy with it, that's good enough for me.

BrianJKoopman · 2026-10-19T13:26:07Z

+        wait_to_start (float): number of seconds to wait between
+            start_time and when the scan actually starts. Default is 10 seconds.
+        step_time (float): time between points on the constant-velocity
+            parts of the motion. Default value is 1.0 seconds. Minimum value is


The default appears to be 0.05 above.

BrianJKoopman · 2026-10-19T13:26:45Z

+        el_endpoint2 (float): second elevation endpoint of the scan.
+            Can be greater than el_endpoint1 for a positive el nod or
+            less than el_endpoint2 for a negative el nod.
+        el_freq(float): frequency of the elevation nods in Hz.


State default.

Co-authored-by: Matthew Hasselfield <mhasself@users.noreply.github.com>

mjrand requested a review from mhasself June 9, 2026 21:28

mjrand self-assigned this Jun 9, 2026

mjrand added the agent: acu label Jun 9, 2026

mjrand marked this pull request as draft June 10, 2026 21:08

mhasself reviewed Jun 11, 2026

View reviewed changes

mjrand force-pushed the ACU_add_sin_el_nod branch from c8dee4c to 6db658d Compare July 1, 2026 01:03

mhasself requested changes Jul 7, 2026

View reviewed changes

mjrand force-pushed the ACU_add_sin_el_nod branch from 748cf3f to b215786 Compare July 7, 2026 01:37

mhasself requested changes Jul 16, 2026

View reviewed changes

This was referenced Aug 21, 2026

seq: Add sinusoidal elevation nod capability simonsobs/sorunlib#264

Merged

Add el nods simonsobs/scheduler#392

Merged

mjrand force-pushed the ACU_add_sin_el_nod branch from 1dbf7e5 to 7ca952c Compare September 1, 2026 20:22

BrianJKoopman added the enhancement New feature or request. label Sep 23, 2026

mhasself approved these changes Oct 7, 2026

View reviewed changes

mhasself marked this pull request as ready for review October 7, 2026 13:22

mjrand added 9 commits October 14, 2026 03:27

Added gen_sin_el_nod function

58553aa

Added sin_el_nod scan type to generate_scan in ACU agent

7a7ebc1

Made generate_sin_el_nod its own agent process

2d88c5d

Updated step_time, generate loop, and various agent bits

865a232

Removed print from drivers generate function

49c31ae

Updated _run_track check for ProgramTrack mode to check track_axes

710d5db

Fixed get_progrtrack checks for multiple axes. Removed initial el_nod…

71f0490

… moves

Fixed nod counter and misc documentation

12bbba3

El freq check and small docstring changes

5aae3b4

mjrand force-pushed the ACU_add_sin_el_nod branch from 7ca952c to 5aae3b4 Compare October 14, 2026 03:45

Removed abs from el freq check

f9be648

mhasself approved these changes Oct 14, 2026

View reviewed changes

Comment thread socs/agents/acu/agent.py Resolved

Comment thread socs/agents/acu/agent.py Resolved

Comment thread socs/agents/acu/agent.py Resolved

mhasself added 2 commits October 14, 2026 08:58

Update socs/agents/acu/agent.py

6b2f83d

Fix typos

7e21c9e

Co-authored-by: Matthew Hasselfield <mhasself@users.noreply.github.com>

mhasself requested a review from BrianJKoopman October 14, 2026 12:59

BrianJKoopman requested changes Oct 19, 2026

View reviewed changes

mhasself reviewed Oct 19, 2026

View reviewed changes

Comment thread socs/agents/acu/agent.py Resolved

mhasself reviewed Oct 19, 2026

View reviewed changes

Comment thread socs/agents/acu/drivers.py Resolved

mhasself reviewed Oct 19, 2026

View reviewed changes

Comment thread socs/agents/acu/drivers.py Resolved

Fix docstrings

ae8f60c

Co-authored-by: Matthew Hasselfield <mhasself@users.noreply.github.com>

mhasself approved these changes Oct 19, 2026

View reviewed changes

mhasself requested a review from BrianJKoopman October 19, 2026 16:28

BrianJKoopman approved these changes Oct 19, 2026

View reviewed changes

mhasself merged commit f4ee7d7 into main Oct 19, 2026
13 checks passed

mhasself deleted the ACU_add_sin_el_nod branch October 19, 2026 16:32

		def check_completed_nods():
		return num_nods is None or t * el_freq < num_nods

		if el_depth == 0:
		raise ValueError("El depth must not be equal to 0 for el nod!")

		el_endpoint2 (float): second elevation endpoint of the scan. For
		constant az scans, this must be equal to el_endpoint1.

Conversation

mjrand commented Jun 9, 2026

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

mhasself commented Jun 10, 2026

Uh oh!

mjrand commented Jun 10, 2026

Uh oh!

mjrand commented Jun 10, 2026

Uh oh!

mhasself left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjrand commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjrand commented Jul 3, 2026

Uh oh!

mhasself left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhasself left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mjrand commented Jul 3, 2026 •

edited

Loading