Detector effects

[soso128]

I added a new feature to the SimpleRate class in order to account for the smearing and detector efficiencies without using GLoBES (simple matrix multiplication). The test suite has been modified accordingly (the simplerate_integration code verified that the computed rates are within 0.1% of the expected rates).

[JostMigenda]

[continuing the conversation here in the main thread, so it doesn’t get hidden when the suggestions above are marked as resolved]

I agree with your suggested changes, included removing the detector_effects option (indeed the smearing and the multiplication by the efficiency array does not take long). Should I implement any of the changes you requested myself or do you just need to commit them in this branch?

For the minor logging changes I’ve suggested, I’ve just made those myself. Removing detector_effects immediately would cause errors if people are using that, though. I’d rather deprecate that first to give users plenty of warning before it is removed. And for removing the SNOwGLoBES class completely, I would wait a bit more for input from others; in case there’s any aspect that the two of us are missing. I’ll bring it up at the SNEWPY dev/user meeting next week and if nobody there sees a problem, whoever of us has time during the hackathon can make those changes.

One remaining concern I have is that the tests associated with SNOwGLoBES do not seem to run (although I did not add new files). I ran simplerate_interface.py locally but not the new version of test_snowglobes. Is there anything I could do to run all checks?

pytest -m 'snowglobes' will run all the tests in test_snowglobes.py. (And if you don’t already have pytest installed, run pip install .[dev] from the SNEWPY repo.)

[soso128]

I just changed the requirement for the new method in test_snowglobes (making it the same as when testing SNoWGLoBES itself, with a 10% error bar). I also modified snowglobes_interface.py to account for missing smearing matrices or efficiency vectors (there are no efficiencies for NOVA for example)

[JostMigenda]

At the SNEWPY user/developer meeting today, we couldn’t think of any issues with removing the SNOwGLoBES class. So let’s go ahead with this, but maybe we can still release that in a beta version first and make sure it gets tested extensively before the final release.

[JostMigenda]

I’ve removed the SNOwGLoBES class, cleaned up unnecessary imports and tests and updated the docs. I think this PR is ready for review now.

Just wondering: Would it be possible to parallelise the loop over flux_files (https://github.com/soso128/snewpy/blob/dd36061b103e19115d6c9778167557d3de5d39cc/python/snewpy/snowglobes_interface.py#L236) or even over channels (https://github.com/soso128/snewpy/blob/dd36061b103e19115d6c9778167557d3de5d39cc/python/snewpy/snowglobes_interface.py#L155) to improve performance?

[JostMigenda]

Some quick benchmarking of snewpy.snowglobes.simulate() using a modified version of SNOwGLoBES_usage.ipynb, “Option 3” with 1/20/40 time bins:

	1 bin	20 bins	40 bins
v1.2.1 (SNOwGLoBES)	0.36s	5.0s	9.7s
this PR (SimpleRate)	2.7s	3.8s	5.0s

So the SimpleRate class needs more time for the initial setup; but is much faster to run on additional flux files even without parallelisation. Looking at the individual steps of the SimpleRate.__init__() function, most of that is spent loading the smearing matrices:

_load_detectors	_load_channels	_load_efficiency_vectors	_load_smearing_matrices
<0.01s	0.05s	0.17s	2.4s

[JostMigenda]

According to a timeit.timeit() benchmark, np.array(list(map(float,line))) is about 2× slower than np.fromiter(line, float); so this little tweak cuts the execution time of _load_smearing_matrices by 0.9s. The result:

	1 bin	20 bins	40 bins
v1.2.1 (SNOwGLoBES)	0.36s	5.0s	9.7s
this PR (SimpleRate)	1.8s	2.9s	4.1s

[JostMigenda]

Another performance tweak: Basically, instead of loading all efficiencies and smearing matrices, SimpleRate now only loads the ones for detectors that the user actually requested. When using a single detector, this shaves another ~1.2 s off the initialisation and the result is:		1 bin	20 bins	40 bins
v1.2.1 (SNOwGLoBES)	0.36s	5.0s	9.7s
this PR (SimpleRate)	0.6s	1.6s	2.8s

It might still be interesting to see if there are further performance benefits to be had by parallelising; but for now, I think the significant performance increase for multiple bins clearly outweighs the extra ~0.2 s for the single-bin case, so I’d prefer to merge this PR as is, instead of leaving it open indefinitely in hopes of further changes. That said, I have now contributed enough to this PR that it would be good if someone else could review it. @sybenzvi @sgriswol, does either of you have a bit of time?