-
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-1104: feature. Add PointingSurvey subclass. #21
Conversation
e9fb975
to
2f4db6d
Compare
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.
Mostly looks okay, except for the need for more detailed docstrings.
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.
For H.A., wouldn't it be more natural to wrap at 12 hours/pi rather than 0? It isn't obvious to me the conditions will always do what you want here (but it isn't obvious it won't). But, if you wrap at 12 hours, and you observe above the pole, then you can just test on a straightforward min < ha < max and it will do what we want.
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.
Yeah, I think I like that better too. I'll change the H.A. in the conditions object to default to -pi to pi. I'll do that on a new branch because there are going to be a number of places where that comes up.
@@ -70,6 +72,60 @@ def __call__(self, observation_list, conditions): | |||
return observation_list | |||
|
|||
|
|||
class FlushByDetailer(BaseDetailer): | |||
""" |
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.
I summary string would be good here.
Altitude limit of the telescope (degrees). Default 85. | ||
ha_max, ha_min : float (4,-4) | ||
hour angle limits (hours). Applied to all observations. Default 4,-4. | ||
weights : dict |
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.
The parameter types from here down need backticks to enable fancy linking when docs are built.
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.
Adding full docstrings to many of these members is important here. When users are exploring what's going on using schedview, there will (eventually) be links to the pages generated from these docstrings to help them understand what's going. One of the big complaints we got from user testing was compaints about the linked-to docs not being helpful enough.
@@ -35,3 +37,24 @@ def dark_sky(nside=32): | |||
dark_sky_data[band] = hp.pixelfunc.ud_grade(dark_sky.data[band], nside_out=nside) | |||
|
|||
return dark_sky_data | |||
|
|||
|
|||
def dark_m5(decs, filtername, latitude_rad, fiducial_FWHMEff, exptime=30.0, nexp=1): |
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.
A more complete docstring would be good here as well.
from rubin_scheduler.scheduler.utils import SkyAreaGenerator, calc_norm_factor_array | ||
|
||
SAMPLE_BIG_DATA_FILE = os.path.join(get_data_dir(), "scheduler/dust_maps/dust_nside_32.npz") | ||
|
||
|
||
class ModelObservatoryWindy(ModelObservatory): |
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.
This seems like something that would be better handled by customizing ModelObservatory rather than a whole new subclass?
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.
I did it as a subclass because I don't think there's a normal use case where we would want a model observatory where it's always windy. Didn't want to add a kwarg that could only ever be used to break things in production.
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.
I think there are use cases where we want to specify specific wind conditions, though, perhaps as an array.
For example, while we've been testing with just one, eventually we will probably want to do small suites of simulations of different weather conditions for the prenight briefing. For example, I will probably want to do a simulation where I give a short arrays of wind speed/direction (probably just one number for each quarter, for one night). So, being able to make an instance of ModelObservatory where I can pass arrays of wind speeds and directions will be something I will eventually want. For the "always windy" case, one could just pass it arrays filled with one number each.
Adding a new
Survey
sub-class for cases where one has a list of pointings to chose from.Examples using the new class over in
rubin_sim_notebooks
Has a stub in for generating DataFrame of it's current state, but will probably need a little refinement before it can drop in seamlessly yo
Schedview
.