search

icecube / pisa

Monte Carlo-based data analysis
http://icecube.github.io/pisa/
Apache License 2.0
19 stars 47 forks source link

Make it possible to weight events #690

Closed JanWeldert closed 1 year ago

JanWeldert commented 2 years ago

Of course events are already weighted, but what I mean is to use a weight per event representing reconstruction uncertainties. In order to do that we have to modify the metric (called weighted_chi2 now) and tell the hist stage to compute the bin uncertainties correctly. In case of no uncertainty weights the weighted_chi2 is identical to the mod_chi2.

JanWeldert commented 2 years ago

btw, I have no idea why so many of my old commits are listed here. Only the last is important

atrettin commented 2 years ago

I'm not sure I understand how the weight represents reconstruction uncertainties... could you please explain what is happening and why? I'm confused why you say that the uncertainty takes reconstruction uncertainty into account, yet when I read the code of the loss function it looks like it is only about the MC uncertainty... which we already have in the mod_chi2 loss. How is weighted_chi2 different from mod_chi2?

JanWeldert commented 2 years ago

You can now add per event uncertainty weights to a container. The difference to the other MC weights is that they don't represent a number of events, but really a weight that would also be included to data in the same way. As a consequence you can't used Poisson errors for the analysis bin contents anymore

atrettin commented 2 years ago

Hm, ok. Is the idea, then, that events with a higher uncertainty get less weight or something to the effect?

JanWeldert commented 2 years ago

Correct, the uncertainty weight could be something like exp(-|uncertainty|) or so

philippeller commented 1 year ago

@atrettin do you agree with this being merged?

atrettin commented 1 year ago

Maaaaybe it would be nice to add at least a small multi-detector example to the unit test of the Analysis class, since that case hasn't been tested before at all. Probably you could just copy&paste from that testing notebook to do it? But if that would be very complicated we don't have to, I would also be ok with merging this.

JanWeldert commented 1 year ago

I can definitely try :)

JanWeldert commented 1 year ago

Does this look good?

atrettin commented 1 year ago

Thank you for getting to this so quickly! Is the test function found and run automatically in our testing pipeline on GitHub? I'm not sure about it, Pytest would probably find it due to the name, but we don't use Pytest for test discovery (which is unfortunate!). I think you need to move the test function into the script that defines the module (pisa/core/detectors.py) so that the test will be found and run automatically.