-
Notifications
You must be signed in to change notification settings - Fork 2
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-1105: bugfix. Ensure twilight near-sun survey executes. #19
Conversation
yoachim
commented
Jan 2, 2024
- Fix bug with nested indexing
- updated unit test to make sure twilight solar system is executing
- update example scheduler to latest
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 88 88
Lines 8844 8843 -1
=====================================
+ Misses 8844 8843 -1 ☔ View full report in Codecov by Sentry. |
Well, it looks like ruff is/was still failing in other places .. I thought I had updated the rubin_scheduler code so that ruff was happy, but it looks like I did that for some pieces of the code but not all. That said, if you're updating this piece of code, it would be good to also make the docstring changes to make ruff and the documentation build happy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for code, but docstrings need a little revision.
|
||
Returns | ||
------- | ||
basis_functions_weights : `list` | ||
list of tuple pairs (basis function, weight) that is | ||
(rubin_scheduler.scheduler.BasisFunction object, float) | ||
(rubin_sim.scheduler.BasisFunction object, float) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rubin_sim -> rubin_scheduler
that can be used with rubin_scheduler.scheduler.schedulers.Core_scheduler. | ||
To ensure we are robust against changes in the sims_featureScheduler | ||
codebase, all kwargs are explicitly set. | ||
This is a convienence function to generate a list of survey objects that can be used with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"convenience" and "rubin_sim" typos
# Generate the rolling footprint for this start of survey | ||
sky = EuclidOverlapFootprint(nside=nside) | ||
# Modify the footprint | ||
sky = EuclidOverlapFootprint(nside=nside, smc_radius=4, lmc_radius=6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smc_radius and lmc_radius -- are these the defaults for EuclidOverlapfootprint? If not, the footprint maybe needs an update?
mjd_start=mjd_start, | ||
sun_ra_start=sun_ra_start, | ||
mjd_start=conditions.mjd_start, | ||
sun_ra_start=conditions.sun_ra_start, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this undo the "use the Almanac instead of the full Conditions" modification, so that we can set up the scheduler without needing skybrightness files immediately?
parser.add_argument("--ddf_season_frac", type=float, default=0.2) | ||
parser.add_argument("--nights_off", type=int, default=3, help="For long gaps") | ||
parser.add_argument("--neo_night_pattern", type=int, default=4) | ||
parser.add_argument("--neo_filters", type=str, default="riz") | ||
parser.add_argument("--neo_repeat", type=int, default=4) | ||
parser.add_argument("--neo_am", type=float, default=2.5, help="airmass limit for twilight NEO visits") | ||
parser.add_argument("--neo_am", type=float, default=2.5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why drop the airmass limit help info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(actually, it would be good to add help statements for all of these)