New option for circular contours with radii sent as parameters #25
Conversation
G-Sommani
changed the title
| def create_plot_zoomed(self, | ||
| extra_ra=np.nan, | ||
| extra_dec=np.nan, | ||
| extra_radius=np.nan, | ||
| systematics=False, | ||
| plot_bounding_box=False, | ||
| plot_4fgl=False): | ||
| plot_4fgl=False, | ||
| circular=False, | ||
| circular_err50=0.2, | ||
| circular_err90=0.7): |
There was a problem hiding this comment.
While this is acceptable to me at this stage, I don't like very much the idea of an ever-growing list of (boolean) arguments in order to control the variant of the plotting.
This leads to a situation where we have 2^n combinations of settings and most of those are not meaningful or have undefined behaviour.
For example, passing circular=True would make the value of systematics irrelevant.
At some point we may want to define the plotting variants in a more structured way (as part of #22 ).
There was a problem hiding this comment.
I also wonder if it's possible to merge the functionality here with the extra_ra/dec/radius arguments? Those are used for plotting the online splinempe circular contours already. With the additional err50/err90 arguments that means it's also possible for err90 < err50 which wouldn't be correct.
There was a problem hiding this comment.
I think it would be nice to keep the option to have both the online splinempe circular contours and other circularized errors at the same time, so I would keep all the arguments. I can add the raising of an error if err90 is smaller than err50.
There was a problem hiding this comment.
Yes, there are several scenario we should cover, I tried to put together a list in #26
The existing extra* arguments allow for a single radius at an arbitrary direction, while here we want two radii at the best fit direction.
I am not sure if we should put effort into merging the two functionalities now considering that all would have to be refactored later.
I guess ultimately we want to achieve something of the kind:
def create_plot_zoomed(self, contours: list[ContourModel], catalogs: list[Catalog])
There was a problem hiding this comment.
Thanks @mlincett for creating the issue. I'm fine with merging this in as it sounds like we're all onboard with improving things later.
tianluyuan
left a comment
tianluyuan
left a comment
There was a problem hiding this comment.
It's nice to see some refactoring. I do wonder if it's possible to merge some of this with the existing extra* arguments, which already plots circular errors.
| def create_plot_zoomed(self, | ||
| extra_ra=np.nan, | ||
| extra_dec=np.nan, | ||
| extra_radius=np.nan, | ||
| systematics=False, | ||
| plot_bounding_box=False, | ||
| plot_4fgl=False): | ||
| plot_4fgl=False, | ||
| circular=False, | ||
| circular_err50=0.2, | ||
| circular_err90=0.7): |
There was a problem hiding this comment.
I also wonder if it's possible to merge the functionality here with the extra_ra/dec/radius arguments? Those are used for plotting the online splinempe circular contours already. With the additional err50/err90 arguments that means it's also possible for err90 < err50 which wouldn't be correct.
| def create_plot_zoomed(self, | ||
| extra_ra=np.nan, | ||
| extra_dec=np.nan, | ||
| extra_radius=np.nan, | ||
| systematics=False, | ||
| plot_bounding_box=False, | ||
| plot_4fgl=False): | ||
| plot_4fgl=False, | ||
| circular=False, | ||
| circular_err50=0.2, | ||
| circular_err90=0.7): |
There was a problem hiding this comment.
Thanks @mlincett for creating the issue. I'm fine with merging this in as it sounds like we're all onboard with improving things later.
ric-evans
left a comment
ric-evans
left a comment
There was a problem hiding this comment.
LGTM -- I agree with the point raised about constraining the number of possible and/or viable configurations. Using pytest could be useful here, see https://docs.pytest.org/en/7.3.x/how-to/parametrize.html
ric-evans
added
enhancement
This modification to the code implements the option to draw circularized errors with radii for 50% and 90% sent as parameters.