Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OPSIM-1175: update the kinematic model and altazshadowmask basis function #88

Merged
merged 20 commits into from
Sep 11, 2024

Conversation

rhiannonlynne
Copy link
Member

I expect this PR could have downstream effects on what is scheduled .. depending on how the observatory kinematic model is set up.
For a fully functioning telescope, with >360 degrees of rotator azimuth range, I don't think it would have that much of an effect. However, it's also true that if we're bumping up against the cumulative azimuth range, this might be an incorrect statement. Depends on the configuration of the AltAzShadowMaskBasisFunction itself during use.
The kinematic model will now return valid slew times over the parts of the sky which are valid, if provided "no filter" or if given rotSkyPos. This again should have no effect on the actual operations of the kinematic model during baseline sim_runner use, but is useful when trying to diagnose issues later or to just investigate possible slew times.

@rhiannonlynne
Copy link
Member Author

I think what's happening with this failure is that the long gaps survey is requesting an out of bounds scripted survey observation. The scripted survey isn't fully checking against the alt/az limits as it's currently set up in the baseline.
I wonder if the only reason this wasn't already failing is that we don't usually run very long without having a fully functional observatory. I don't know why the unit test passed previously though .. the alts within the scripted survey aren't picking up the new limits when set up how it is now, so I must need to figure out how to pass them into the scripted survey. I'm inclined to add the AltAzShadowMask function as a default mask for the scripted survey.

@rhiannonlynne rhiannonlynne force-pushed the tickets/OPSIM-1175 branch 5 times, most recently from dd539c5 to cd1c448 Compare August 29, 2024 22:51
@rhiannonlynne
Copy link
Member Author

I think what's happening with this failure is that the long gaps survey is requesting an out of bounds scripted survey observation. The scripted survey isn't fully checking against the alt/az limits as it's currently set up in the baseline. I wonder if the only reason this wasn't already failing is that we don't usually run very long without having a fully functional observatory. I don't know why the unit test passed previously though .. the alts within the scripted survey aren't picking up the new limits when set up how it is now, so I must need to figure out how to pass them into the scripted survey. I'm inclined to add the AltAzShadowMask function as a default mask for the scripted survey.

This problem was indeed fixed when previous PRs added the AltAzShadowMask to the scripted survey and cleaned up the note handling.

Copy link
Member

@yoachim yoachim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation for refactoring alt/az limits is kind of mysterious. There's not a bug being patched, there's not a performance boost to be had, there's no new functionality being added (and some potentially lost).

If we want to be able to communicate limited telescope functionality to the scheduler, I would just put that information into the Conditions object and have a basis function that is dedicated to interpreting it and generating the correct mask. There's no need to overload the slew time map.

rubin_scheduler/scheduler/sim_runner.py Outdated Show resolved Hide resolved
rubin_scheduler/scheduler/model_observatory/kinem_model.py Outdated Show resolved Hide resolved
@rhiannonlynne
Copy link
Member Author

rhiannonlynne commented Sep 5, 2024

Summarizing a live conversation about AltAzShadowMask:

  • one goal is that the AltAzShadowMask should be usable to set preferred regions of sky, by the user, as kwargs
  • the second is that the AltAzShadowMask should be able to be configurable to know about (possibly dynamic) limitations from the telescope.

I had tried to get the limitations from the telescope passed as part of the limitations from the telescope configuration - kinematic model, in this case, or observatory model in operations, and figured these were both constraints about physical limitations for the mount (as in, you couldn't even pass through these regions).
In practice, these may be more complex -- regions you can pass through but don't want to point to (there is a light that is too bright in this direction or ?), vs. we really can't slew through this region at all.
Specifying these different kinds of limits and passing them along lies within the lsst-ts packages, and may not be thoroughly defined yet. (TBD with @tribeiro).

So we have:

  • kwargs for alt/az regions the user doesn't want to include (these should go back to lists of lists, most likely, so that you can have disjoint regions)
  • conditions values for places that should not be pointed at (again, lists of lists, so you can have disjoint regions)
  • limitations for the telescope so that we don't slew into places on the sky that are inaccessible. These should also be passed along to the conditions values above, but may need to be considered separately. (maybe just actually access slew times to mask out places of the sky which are not available?).

I think this means the kinematic model updates remain the same, the AltAzShadowMaskBasisFunction needs updates, and the way the ModelObservatory passes values to the Conditions (and then the AltAzShadowMaskBasisFunction) needs updates (probably update to something similar to what the previous handling was?).
(update: this is more how the final PR looks).

…onditions objects. Now will use most restrictive of either keyword or conditions values (so default conditions are widely set).
Modify use of filters within kinematic model (slewtime can be calculated in any filter, filter will be set to one of allowable filters when parked).
Conditions object gets azimuth limits from azimuth limits of model.
AltAzShadowMaskBasisFunction adds padding for azimuth limits from telescope model.
RotSkyPos values out of range are only masked in the parts of the sky where they are out of range.
Add dictionary of tma/rotator values that can be set via “percent of max performance”.
Squashed commits:
[4611e38] Update unit test parameters and error message, plus comments
isort (+1 squashed commit)
Squashed commits:
[dd539c5] Mark alternative zenith or azimuth masks as to be deprecated.
Keeps previous capability of allowing disjoint altitude or azimuth regions, while also respecting kinematic model constraints
Added more extensive unit test
…de them.

Keith expects to modify this over the course of commissioning, and that the field centers may not align with full LSSTCam field centers anyway, so hard-coded is likely easier to see and track.
@rhiannonlynne
Copy link
Member Author

After the changes, there are basically discontiguous areas which are (optionally) passed from the observatory model. The AltAzBasisFunction will also pick up limits from the telescope model automatically (which currently isn't a problem in our standard simulations, as we don't degrade the telescope performance .. but might be more of an issue as we evaluate commissioning or early science surveys where the altitude range in particular will be limited).

The kinematic model is more thoroughly separated from this than previously, in that it doesn't even carry the optional limits now. The slewtime calculation has been updated so that it is possible to calculate slew times with a compromised azimuth range though (previously the kinematic model would return infinite slew times over the whole sky if the available azimuth range was less than 360 degrees).

There is much more extensive testing of the AltAzShadowMask and some other basis functions.

@rhiannonlynne
Copy link
Member Author

rhiannonlynne commented Sep 10, 2024

The problem that this PR addresses is to make it easier to avoid choosing pointings that are in inaccessible regions, without having to set alt/az limits carefully for each survey -- as well as letting them be updated easily via the conditions object using conditions.kinematic_az_limits and conditions.kinematic_alt_limits, which @tribeiro can pass from
the configuration of the observatory model (lsst-ts.ts_observatory_model) into the conditions object in the Driver. Or he could pass them into conditions.tel_az_limits and conditions.tel_alt_limits if they are not necessarily appropriate for restriction slew areas but are appropriate for restricting general choices for observing (such as if the AOS system needs to limit the telescope to a given altitude range during commissioning) (presumably these will also be in a yaml file somewhere).

If a survey is configured with an AltAzShadowMaskBasisFunction, it will then pick up these restrictions automatically and use these to mask parts of the sky which should be inaccessible.

The AltAzShadowMask adds conditions.altaz_limit_pad around these constraints, to help avoid choosing pointings which the healpix reward map states are accessible (such as from the SlewTimeBasisFunction). The mapping from healpix reward map to pointing can lead to pointings which are ever so slightly outside the reward map - the altaz_limit_pad helps avoid this.
It's true this can be configured through the surveys with altitude limits, but this makes it easier to pick up the constraints automatically without having to reconfigure multiple surveys.

The screenshots below show a simple greedy survey configured with and without the AltAzShadowMask basis function, which would get the kinematic limits of the telescope (here unrestricted, just normal altitude limits). The background healpix map is the slewtime map - gray areas are out of bounds. The red dots are the pointing centers which the reward map would include as valid field centers.

The first is without the AltAzShadowMaskBasisFunction and the second is with the AltAzShadowMaskBasisFunction. The differences are subtle, but can be seen at the high altitude limits near zenith (note there is also a 'shadow_minutes' set which extends the high-altitude mask into the area which is soon to be in the zenith avoidance region).

Screenshot 2024-09-10 at 11 27 35 AM Screenshot 2024-09-10 at 11 29 42 AM

@rhiannonlynne
Copy link
Member Author

rhiannonlynne commented Sep 10, 2024

The differences seen in the final unit test which is failing seem to be minor. The difference lies in whether or not the pairs 33, ri, b scheduler note is included or not -- with this branch, it is not but in main, it is. There are about 50 observations with this note on main, while here there are none. This is not a negligible number of visits, out of the ~1500 that are acquired over the course of the 3 days included in the unit test. The number of "a" pairs for this blob is also fewer in this branch (19) vs. on main (69) and the overall number of visits is fewer as well (25 less), due to a slightly longer slewtime.
Interestingly, the scheduler_note pairs 33, ri, b can be restored to the output if a previous PR which changed the BlobSurvey ordering from being based on alt/az into ra/dec is reversed. This change was indicated to be not significant previously.

With the previous PR indicated as not a significant change, I'm inclined to simply remove the requirement for having pairs 33, ri, b in the output list of scheduler_notes for the unit test test_utils::TestUtils:test_example.

@rhiannonlynne
Copy link
Member Author

For completeness's sake - you can also use the AltAzShadowMaskBasisFunction to restrict the area available on-sky via the arguments for the basis function itself:
bfnew = basis_functions.AltAzShadowMaskBasisFunction(min_alt=0, max_alt=90, min_az=270, max_az=90)
results in available pointings (for the same survey at the same time, but modifying the basis functions):

Screenshot 2024-09-10 at 11 47 21 AM

or
bfnew = basis_functions.AltAzShadowMaskBasisFunction(min_alt=70, max_alt=90, min_az=0, max_az=360)

Screenshot 2024-09-10 at 11 49 00 AM

or (by reducing the shadow_minutes, which adds a mask in areas which are expected to land within the inaccessible region at time = shadow_minutes + current_time) -
bfnew = basis_functions.AltAzShadowMaskBasisFunction(min_alt=70, max_alt=90, min_az=0, max_az=360, shadow_minutes=0)

Screenshot 2024-09-10 at 11 49 50 AM

Copy link
Member

@yoachim yoachim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine on a 5-year run.

@rhiannonlynne rhiannonlynne merged commit ce66ed1 into main Sep 11, 2024
7 checks passed
@rhiannonlynne rhiannonlynne deleted the tickets/OPSIM-1175 branch September 11, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants