SNEWS2 / snewpy

A Python package for working with supernova neutrinos
https://snewpy.readthedocs.io
BSD 3-Clause "New" or "Revised" License
26 stars 19 forks source link

`Fornax_2021.get_initial_spectra()` performance improvements and bug fixes #166

Closed JostMigenda closed 2 years ago

JostMigenda commented 2 years ago

Previously, Fornax_2021’s __init__() function would open the input file, read some data from it and then keep it open and read from it again in get_initial_spectra(). This PR changes this to read all required data from the file exactly once (in __init__ ()).

The result (measured by putting a timeit.default_timer() at the beginning/end of each function):

Since __init__() only gets executed once, but get_initial_spectra() may get executed many times (e.g. once per time bin when generating SNOwGLoBES input files), this may make a significant difference in practice.

A nice side effect (and my main reason for looking into this in the first place): Keeping the open file as an instance variable meant that Fornax_2021 objects couldn’t be pickled, which meant they couldn’t be used in a multiprocessing context, which e.g. sntools uses.

JostMigenda commented 2 years ago

I’ve added a commit which returns number luminosity spectra (instead of luminosity spectra), like all other models. This fixes #167; see that issue for details.

Please note: For interpolation='nearest' I think I’ve discovered a bug in the treatment of energy bins—a simple mix-up between bin centres and edges. I’ve fixed that in a separate commit and made the variable names and comments a little clearer.

@sybenzvi, since you originally added the Fornax models, could you review this PR?

github-actions[bot] commented 2 years ago

Result of Benchmark Tests

Benchmark Min Max Mean
python/snewpy/test/test_snowglobes.py::test_simulation_chain_benchmark 5.20 5.30 5.25 +- 0.04
sybenzvi commented 2 years ago

I'm fine with these changes so I'll merge the PR.