-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation of NeutrinoFlux container #236
Conversation
Applying |
I've added a notebook describing the usage and main features of the new flux.Container class. |
The base functionality is implemented, and before I continue, I'd like to have opinions on the interface I propose.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work @Sheshuk! The notebook is a nice illustration of the functionality and I think the overall direction is good.
There are a few conceptual issues I have with the Container.sum
and Container.integrate
functions.
Regarding Container.sum
:
- Summing along the flavor axis is okay; though the use cases for that are fairly limited, I think.
- Summing along the time or energy axis of a
Flux
is physically meaningless: It gives results that depend on the binning in the original model file—especially so, since several model files have non-uniform binning, e.g. very narrow sub-ms time bins during the accretion phase and then wider time bins (up to ~100 ms) during the neutrino cooling phase. Simply summing those different bins without accounting for bin widths gives completely arbitrary numbers. - Summing over the time axis of a
Fluence
or the energy axis of aSpectrum
is meaningful and may be useful iff the integration that created thatFluence
/Spectrum
used >2 entries inlimits
.
Regarding Container.integrate
:
- Integrating over the flavor axis doesn’t really make physical sense; and unfortunately, numerically it’s not the same as summing over the same flavours.
- Integrating along an axis that was already integrated should probably return an error.
I suspect all of these could be fixed by careful checks at the start of the sum
and integrate
functions; but I haven’t fully thought through the implementation details.
Other than that, I’ve suggested a few minor changes to the code inline.
Oh, and especially nice work on the ContainerClass
helper function—that’s a very elegant way to solve this issue! 🤩
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
For now, that’s definitely a good idea. Since this is intended to be temporary, I haven’t looked at the Before we can merge
Since
Agree; the unweighted numbers are an intermediate step and I don’t see any use case for exposing them to the user. |
@JostMigenda thank you for your review and suggestions! About the integration/summation - I totally agree, in some cases it doesn't have physical sense. I thought that it's up to a user to choose physically meaningful operations, but having checks would be nice. I need to think about how to implement it. Probably keeping a set of
You're correct - if the integration bins are the same as the initial energy sampling points. But the sampling points can be much more detailed - this is especially the case I want for preSN models - while the integration bins are defined by the SNOwGLoBES smearing matrix bins. |
I have:
So now most other tasks were finished or postponed, I think I'm ready to work on this. |
Thanks for looking into this!
Right, it was introduced in ee69beb - it looks like we were treating the SNOWGLOBES binning incorrectly all this time, but the tests are still for the old version. |
…defined in SNOwGLoBES config" This reverts commit ee69beb.
…ctly as defined in SNOwGLoBES config"" This reverts commit f4cc7ea.
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
@Sheshuk and I have talked about this PR and #251 earlier today and fixed some remaining bugs. Since both PRs are closely related, it makes sense to first merge #251 (where these SNOwGLoBES binning issues & related tests are both fixed) into this one, and then merge this PR here into the main branch. For the record, the final commit on this PR, before the first merge, was a42ac54 And aside from the functional reasons for this PR, which we’ve discussed in detail, I’d just like to note that there are some nice performance benefits as well: In my testing using the example in |
Implementing `snewpy.snowglobes` functions using `flux.container` class
Some more issues with the integration tests. The first one was because of a tiny change in the keys for the results dictionary; that’s fixed now. The current issue requires a bit more thought, because this PR changes the binning and that leads to slightly different event counts: Unsmeared:
The unsmeared numbers typically change by less than 1 per mille; I think that’s nothing to worry about. Smeared:
After smearing², the changes are much bigger, mainly in the electron scattering and IBD channels. In water Cherenkov detectors, those are the two channels which have the most events near the detector threshold (~5 MeV or so, depending on the detector); so any changes to the binning that cause events to slip above/below the detector threshold would predominantly affect those two channels. So I think these changes are within what we’d expect; but maybe we should wait a little to let people comment? I’ll also try to double-check the binning code tomorrow, just to feel confident I didn’t miss anything. ¹ This is the sum of the rows above, multiplied by 0.32 (because the SNOwGLoBES detector config has 100 kt volume, while SK’s inner detector has 32 kt). |
@JostMigenda Are the percentages off by a factor of 10? edit: I see, per mil, a weird unit … |
@evanoconnor Changes are all in per mille, not per cent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did go through the rate calculation code once again; thanks to @Sheshuk for pointing me to pp. 100&105 of the GLoBES manual for more on the energy binning and smearing files.
It looks good to me and I would be happy to merge this in a moment; I’ll just need to submit a quick commit to fix the number of events required in the integration tests first.
See discussion on #236 for context
Closes #214
Initial tasks (before finalizing the interface)
SupernovaModel.get_flux
should produceFlux
class objectSimpleRate
or similar class)save/load
functions for this class (to do the caching, if desired)Next tasks:
Postponed tasks
Implement
Flux.apply(flavor_xform:FlavorTransformation)
method, to apply the transformation, and produce a transformed fluxPostponed till #242 is implemented, right now
SupernovaModel.get_flux
gets aFlavorTransformation
argument to have transformed flux)Merge
RateCalculator
andSimpleRate
classesI separated the common part as
SnowglobesData
base class, which handles the reading and accessing stuff from the snowglobes. So I think there is no need for this merge