Changes to visit map function to allow for multiple night data #175
Conversation
ehneilsen
left a comment
ehneilsen
left a comment
There was a problem hiding this comment.
In general, this looks good: most comments are either about following the Rubin style guide (here: https://developer.lsst.io/ ) or maintaining backward compatibility.
For branch names, we should be following the Rubin style guide here: https://developer.lsst.io/work/flow.html#git-branching . In particular, in the future, let's make a Jira issue and corresponding branch name for any PRs.
Pulling _add_visit_patches, _add_celestial_objects, and _setup_slider_callback out into helper functions is a great improvement.
One way of maintaining backward compatibility (assumed by the detailed comments below) requires making a couple of branches in each of the functions, but I think it isn't so bad (yet).
An alternative for maintaining backward compatibility and still avoiding code duplication is to go even further with pulling out helping functions from the original functions, then instead of your altering existing functions to make your multi-night versions, make entirely new functions (maybe create_multinight_visit_skymaps and plot_multinight_visit_skymaps) that call the same set of helper functions: if most of the "work" is done in the helper functions, that would also avoid excessive code duplication.
I think there are few enough code branches at present that either approach is reasonable: whatever you like better is fine with me.
| applet_mode=False, | ||
| ): | ||
| """Plot visits on a map of the sky. | ||
| """ |
There was a problem hiding this comment.
The docstring should continue to follow the convertions in the standards page, with types in back-ticks so they get processed properly when the docs are made.
| 'fieldRA', 'fieldDec', 'band' | ||
| footprint : np.array or None | ||
| Healpix footprint | ||
| conditions_list : list |
There was a problem hiding this comment.
Can we have it take either a single conditions object or a list, such that it is backwards compatible? Something like
conditions_list = [conditions] if isinstance(conditions, Conditions) else conditions
early on in the body of the function should do it.
| if footprint_outline is not None: | ||
| add_footprint_outlines_to_skymaps( | ||
| footprint_outline, spheremaps, line_width=5, colormap=defaultdict(np.array(["gray"]).item) | ||
| footprint_outline, spheremaps, line_width=5, colormap=defaultdict(lambda: "gray") |
There was a problem hiding this comment.
The style guide does not allow the use of lambda (see here), so we should go back to the weird construction I used originally.
Yes, I think using lambda makes more sense, but we should follow the style guide.
| fig : `bokeh.models.layouts.LayoutDOM` | ||
| A bokeh element that can be displayed, or incorporated into a larger | ||
| element. | ||
| visits : pd.DataFrame |
There was a problem hiding this comment.
For backwards compatibility, it would be good if the function could take visits DataFrames without day_obs.
Fortunately, day_obs can be computed from observationStartMJD, so this is possible by putting this at the start of the function body:
if "day_obs" not in visits.columns:
def mjd_to_day_obs(mjd):
return int((datetime(1858, 11, 16, 12, 0, 0) + timedelta(days=mjd)).strftime("%Y%m%d"))
visits = visits.assign(day_obs=visits.observationStartMJD.apply(mjd_to_day_obs))
| mjd_slider = spheremaps[0].sliders["mjd"] | ||
| mjd_slider.start = mjd_start | ||
| mjd_slider.end = mjd_end | ||
| mjd_slider.value = mjd_start |
There was a problem hiding this comment.
If the default value of the slider is mjd_start, then the initial display is with no visits displayed (because you're only showing visits before the start). I think it would make sense to make the default slider value mjd_end, so that all visits are shown when the plot is first displayed.
|
|
||
| def create_visit_skymaps( | ||
| visits, | ||
| night_date, |
There was a problem hiding this comment.
For backwards compatibilit, night_date needs to continue to be a parameter, but we can set it to None by default (in which chase all dates can be included).
| planisphere_only=False, | ||
| applet_mode=False, | ||
| ): | ||
| """Create a map of visits on the sky. |
There was a problem hiding this comment.
This function should have a full docstring (can use the previous one with minor adjustments).
| The arguments used to produce the figure using | ||
| `plot_visit_skymaps`. | ||
| """ | ||
| site = None if observatory is None else observatory.location |
There was a problem hiding this comment.
If night_date is not None, we should continue to apply these lines for backwards compatibility.
Included in this would be continuing to support passing a path for the visits, in which case the visits are loaded with read_opsim.
Changes to visit map function to allow for multiple night data