tickets/PREOPS-5023: implement sky maps with skyproj in maf #403
Conversation
| "skyproj": skyproj.MollweideSkyproj, | ||
| "skyproj_kwargs": {"lon_0": 0}, | ||
| "decorations": self.default_decorations, | ||
| } |
There was a problem hiding this comment.
I think there are some default kwargs that are expected to be present, for other plotters ... the BasePlotter suggests
self.default_plot_dict = {
"title": None,
"xlabel": None,
"label": None,
"labelsize": None,
"fontsize": None,
"figsize": None,
}
although, this is really only weakly required, and is mostly because when filling out the plotters, tests are not always done to ensure those keys are in the dictionaries.
I see you tested for title before using it and same with title, so it's also not necessarily relevant in this case.
I suppose making the list of plot dict items more standardized and more explained would be helpful. (I can't remember offhand, if "label", for instance, is used to make the color-bar label or if it's xlabel, for skymaps).
There was a problem hiding this comment.
I think it may be useful to add either a comment or to expand the default plot_dict to include the optional items --
"ModelObservatory", "labelsize", etc.
Otherwise it's really hard to know what those keys are .. basically the default_plot_dict lets you have a place to look it up.
And then you can just test if it's None rather than testing if it's present and if it's None.
There was a problem hiding this comment.
Okay, I've added these to the defaults, and added more description to the comment about what decorations need ModelObservatory and which don't.
| This method relies on the site location and time (as an mjd) set in | ||
| the ``model_observatory`` element of the ``plot_dict``, which | ||
| should be of the class | ||
| `rubin_scheduler.scheduler.model_observatory.ModelObservatory`. |
There was a problem hiding this comment.
It would be great if you did not need ModelObservatory to add the ecliptic and galactic plane.
There was a problem hiding this comment.
It seems like maybe you don't, and in fact, if you don't add ModelObservatory to the plotdict, then this just works as expected - adding ecliptic and galactic plane, but not the sun/moon?
There was a problem hiding this comment.
Indeed, ModelObservatory is only needed for decorations that actually need it. I've expanded the comment to make this clearer.
|
|
||
| # This masking replicates the behavior of HealpixSkyMap, but is it | ||
| # what we really want to do? Why not use the default handling of | ||
| # masked values applied by skyproj? |
There was a problem hiding this comment.
I think here, "mask_below" was different than "a metric value which was masked".
In general, I don't use "mask_below" and I think it was added by @yoachim but I am not sure if there is still a need for it?
I guess what you're saying here, is why not set the HealSparse 'sentinel values' to the mask_below limit, instead of just hp.UNSEEN?
| "skyproj": skyproj.MollweideSkyproj, | ||
| "skyproj_kwargs": {"lon_0": 0}, | ||
| "decorations": self.default_decorations, | ||
| } |
There was a problem hiding this comment.
I think it may be useful to add either a comment or to expand the default plot_dict to include the optional items --
"ModelObservatory", "labelsize", etc.
Otherwise it's really hard to know what those keys are .. basically the default_plot_dict lets you have a place to look it up.
And then you can just test if it's None rather than testing if it's present and if it's None.
No description provided.