search

icecube / skyreader

An API for Results Produced by SkyDriver & the Skymap Scanner
MIT License
0 stars 0 forks source link

Mock ts maps for circularized contours and neutrino floor implementation. #35

Open G-Sommani opened 2 months ago

G-Sommani commented 2 months ago

This pull request would introduce two main changes/new implementations:

  1. Mock ts maps for circularized contours. A new parameter, circularized_ts_map, is now introduced. As default it is False, but if set to True it produces a circularized ts map with a symmetric bi-variate Gaussian shape and centered on the best-fit direction. The standard deviation of the bi-variate Gaussian is estimated from circular_err90 such as the contour with 90% coverage is at a radius equal to circular_err90.
  2. Neutrino floor implementation. Also in this case there is a new parameter, neutrino_floor, by default set as False. If set into True it checks the area of the 90% contour. If it is smaller than what expected in case of neutrino floor (as implemented offline, a bi-variate Gaussian with sigma=0.2 degrees), it sets circularized_ts_map to True and circular_err90 to the correct value for having sigma=0.2 degrees.
G-Sommani commented 1 month ago

@ric-evans I have moved now 4 functions from plot.py to plotting_tools.py. These functions serve to calculate areas or estimating the angular distance of pixels, so not really directly for plotting. Do you think it would be better to move them to a dedicated file?

ric-evans commented 1 month ago

@ric-evans I have moved now 4 functions from plot.py to plotting_tools.py. These functions serve to calculate areas or estimating the angular distance of pixels, so not really directly for plotting. Do you think it would be better to move them to a dedicated file?

In terms of lines per file, this looks much better. I personally keep to a max of 1000 lines and consider splitting at ~700.

As for how to group functions, you're the expert so whatever you think is best! Files are free, so there's no need to wait until modules reach X lines to split. Really, it's best to avoid splitting in the first place (from the POV of the main branch) since git line history will be lost for the relocated lines. *_tools.py modules do naturally tend to bloat over time since they can serve as a catch-all.

tianluyuan commented 1 month ago

Im out of town right now and will try to give this a more careful look when I get back. A quick suggestion if we wanted to fix the area calculation bug would be to split the PR up so that 1 can be merged in since that is a bugfix. 2 and 3 maybe requires a bit more thought. This package is meant to handle the output of skymap scanner and so it would be nice to have a well defined scope.

G-Sommani commented 1 month ago

Not from the PR: I'm concerned that this line https://github.com/icecube/skyreader/blob/ts-map-rude-events/skyreader/plot/plotting_tools.py#L320 will be forgotten about. Would it make sense to move it elsewhere, or is this a common pattern? I know matplotlib has anti-patterns (like https://github.com/icecube/skyreader/blob/ts-map-rude-events/skyreader/plot/plotting_tools.py#L19) that I may just need to get over 😆

I moved that line inside the __init__ function of the class SkyScanPlotter (which handles the plots). It seems necessary to use the AstroMollweideAxes class defined in plotting_tools.py. I guess it makes sense to register this class as soon as the SkyScanPlotter is initialized.

G-Sommani commented 1 month ago

I created a new file areas.py with two functions, calculate_area and get_contour_area, to estimate the areas of contours. I left get_space_angles inside plotting_tools.py as it serves to get a specific property (the angular distance of pixels) useful for plotting. And I moved log_gauss back in plot.py. This function would serve as a sort of psf model for the circularized contours.

Regarding @tianluyuan's comment, the circularized contours would always be pointed towards the direction of the minimum, so the output of the skymap scanner is always essential here and has to be handled to have the circularized contour. The option to apply a neutrino floor (which requires the possibility of generating circularized contours) may be useful with any reconstruction as 0.1 degree precision could still be affected by systematics as the geometry of the detector.

tianluyuan commented 1 month ago

Maybe there are three main issues.

G-Sommani commented 1 month ago

Maybe there are three main issues.

* Technically, there is an api to access the contour areas and skymap-scanner best fits. Using this api, isn't it possible to do the mock up skymap externally?

* Such an external tool would also be useful since the mock up skymap does not necessarily need to be centered on the skymap-scanner's best fit. One can then circularize errors about any minimum, and build the map around that.

* I'm not sure there's a strong scientific argument to circularize errors when we have the full likelihood map available. In practice, I think something like a recalibration of the likelihood levels could be a more useful feature i.e. a way to assign probability levels based on different delta-llh values, generalizing from chi2(k=2).

Hi @tianluyuan, thank you for telling me about this api. Could you point me to it? I see no problem in implementing circularization and neutrino floor there. Regarding your last point, two considerations:

  1. do you mean giving the user the possibility of giving the llh levels as an input? Or skyreader recalibrates these levels?
  2. Instead of the circularized error, the user could give as an input a minimal required area, and skyreader increases the likelihood levels until this area is reached. At this point I guess the question would be if both 50% and 90% areas should be specified, or if only one of the two areas should be specified and then the other one obtained as a consequence (like the ratio between the two areas in a bidimensional symmetric gaussian).