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).

… moves

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...)

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 added 7 commits July 7, 2026 01:06

Added gen_sin_el_nod function

1f660d5

Added sin_el_nod scan type to generate_scan in ACU agent

4a03c0b

Made generate_sin_el_nod its own agent process

55f4526

Updated step_time, generate loop, and various agent bits

576486a

Removed print from drivers generate function

d0f6a58

Updated _run_track check for ProgramTrack mode to check track_axes

0e71138

Fixed get_progrtrack checks for multiple axes. Removed initial el_nod…

b215786

… moves

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

Fixed nod counter and misc documentation

1dbf7e5

This was referenced Aug 21, 2026

Add sine_el_nod simonsobs/sorunlib#264

Open

Add el nods simonsobs/scheduler#392

Open

		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!")

Uh oh!

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!

mjrand commented Jul 3, 2026 •

edited

Loading