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

SP-1841: Backwards compatible updates for Band/Filter #149

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

rhiannonlynne
Copy link
Member

This PR has the goal to make the move from "filter" to "band" backwards compatible.
This lets us tag at 3.5.0, making the band update a "feature" rather than breaking change, and lets @tribeiro deploy this at the summit without having to make over ts_scheduler.
I've tested this with the current develop of ts_fbs_utils and the unit tests there run. I've also tested it with sim_runner and our baseline survey configuration.
I am unsure of how to test this in a full run with ts_scheduler to ensure there are no breakages (this is technically true for any update we make).

It would be good if both @tribeiro and @yoachim could take a look.
The main place I am concerned about is in the Conditions object and in add_observation.

  • in the Conditions: ts_scheduler currently reaches into the conditions object and adds information to relevant fields one-by-one. In this call, the "mounted_filters" and "current_filter" are set, using the physical filternames. When using the modelobservatory, both the current_band and current_filters are set directly using the band name.
    With this PR, I have changed mounted_filters and current_filter to be set using a setter, so that if current_filter is set, then current_band is updated (splitting physical filtername down to band) and likewise for mounted_filters. Thus setting mounted_filters or current_filter will update the mounted_bands and current_band (which are the values which are then used inside the remainder of the FBS when referring to conditions values).

  • in add_observation: I am assuming that incoming observations from ts_scheduler will have 'filter' filled out (which could even be the full physical filter name) and will have 'band' as a field available to be filled, but that it won't have any information -- so then add_observation has a small "feature" at present to fill out 'band' if it wasn't already present (and should, in general split physical filter names properly down to band). With sim_runner, 'band' will already be filled out and so nothing will happen.

@rhiannonlynne rhiannonlynne requested a review from yoachim January 19, 2025 22:25
@@ -182,6 +182,8 @@ def __init__(self, scheduler_note=None, bandname=None, note=None):
if self.scheduler_note == "":
self.scheduler_note = None
self.bandname = bandname
# Will not add filtername backup here; users should reach
# down as far as the features in configurations
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a missing "not" in here? Should be something like "users do not set feature.init kwargs when configuring a scheduler obejct"

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.

Makes sense.

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 38.21656% with 97 lines in your changes missing coverage. Please review.

Project coverage is 78.45%. Comparing base (7615c3f) to head (bcae331).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...duler/scheduler/basis_functions/basis_functions.py 33.92% 28 Missing and 9 partials ⚠️
rubin_scheduler/scheduler/surveys/surveys.py 0.00% 9 Missing and 4 partials ⚠️
rubin_scheduler/scheduler/utils/footprints.py 16.66% 6 Missing and 4 partials ⚠️
rubin_scheduler/scheduler/surveys/too_surveys.py 10.00% 9 Missing ⚠️
rubin_scheduler/scheduler/detailers/detailer.py 41.66% 6 Missing and 1 partial ⚠️
rubin_scheduler/scheduler/surveys/field_survey.py 14.28% 4 Missing and 2 partials ⚠️
...ler/scheduler/basis_functions/feasibility_funcs.py 62.50% 3 Missing ⚠️
...heduler/scheduler/basis_functions/rolling_funcs.py 0.00% 2 Missing and 1 partial ⚠️
...ubin_scheduler/scheduler/detailers/short_survey.py 25.00% 3 Missing ⚠️
...in_scheduler/scheduler/surveys/scripted_surveys.py 25.00% 2 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
- Coverage   78.74%   78.45%   -0.30%     
==========================================
  Files         150      150              
  Lines       15354    15464     +110     
  Branches     1773     1798      +25     
==========================================
+ Hits        12091    12132      +41     
- Misses       2777     2823      +46     
- Partials      486      509      +23     

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

@rhiannonlynne rhiannonlynne merged commit abe820a into main Jan 21, 2025
9 of 11 checks passed
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.

None yet

2 participants