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

snowglobes.SimpleRate: add method for rate calculation from the given flux array #216

Closed Sheshuk closed 1 year ago

Sheshuk commented 1 year ago

What I want

To be able to calculate the rate from the neutrino flux stored as np.array in memory:

sng = SimpleRate()
result = sng.compute_rates(flux_array, energies, detector, material)

What we have

There are only methods for calculating rates reading the flux files from the disk: https://github.com/SNEWS2/snewpy/blob/b35379952fe18aedc28cdb106f1ee9e7ad08b082/python/snewpy/snowglobes_interface.py#L156 https://github.com/SNEWS2/snewpy/blob/b35379952fe18aedc28cdb106f1ee9e7ad08b082/python/snewpy/snowglobes_interface.py#L205

What I suggest

A small refactoring inside SimpleRate class:

  1. Transform
    _compute_rates(self, detector, material, flux_file:Path)

    to

    compute_rates(self, flux:np.ndarray, energies:np.ndarray, detector, material)
    #Note: also making the function public

    and move reading the arrays from files to SimpleRate.run

mcolomermolla commented 1 year ago

I like the suggestion, but this would request using something different from compute_fluence as a start point, and getting directly those time and energy arrays at the output of the function, which are then passed to compute_rates in the next step.

JostMigenda commented 1 year ago

Right now, the module level docstring of snewpy.snowglobes_interface has a note saying:

Users should only use the high-level interface described above. This low-level interface is not guaranteed to be stable and may change at any time without warning, e.g. to support new SNOwGLoBES versions.

So if the changing _compute_rates to take a flux array is useful to implement other changes to snewpy (e.g. suggested in #213 or #214), I’m fine with that. But I’d be very hesitant to make any parts of the module public, at least in the short term. I’d rather keep that private until we’ve made other changes and are reasonably certain that the SimpleRate class and its implementation are unlikely to change.

Sheshuk commented 1 year ago

But I’d be very hesitant to make any parts of the module public

You didn't hesitate at all substituting snowglobes_interface.SNOwGLoBES --> snowglobes_interface.SimpleRate, and now you're worried by snowglobes_interface.SimpleRate._compute_rates --> snowglobes_interface.SimpleRate.compute_rates?

By public I don't mean declaring this interface as recommended. I just would like to get rid of the underscore, to avoid calling underscored methods from outside of the module later.

Sheshuk commented 1 year ago

I like the suggestion, but this would request using something different from compute_fluence as a start point, and getting directly those time and energy arrays at the output of the function, which are then passed to compute_rates in the next step.

No, I don't suggest to change the SimpleRate.run function, which operates on one of more flux_files. I just suggest to move the reading of these files from compute_rates to run method, so that compute_rates would be called inside run, but also could be used apart from it. This doesn't change anything for the outside users/methods (snowglobes.simulate will work absolutely the same)

JostMigenda commented 1 year ago

Okay, I misunderstood you there. If it makes sense to use compute_rates elsewhere within snewpy that’s fine; and feel free to drop the underscore as well. I just don’t want anything in snowglobes_interface to be public (in the sense of: letting users access it directly in their notebooks/scripts); otherwise, as you say, the move to SimpleRate would have been much more complicated.

mcolomermolla commented 1 year ago

I like the suggestion, but this would request using something different from compute_fluence as a start point, and getting directly those time and energy arrays at the output of the function, which are then passed to compute_rates in the next step.

No, I don't suggest to change the SimpleRate.run function, which operates on one of more flux_files. I just suggest to move the reading of these files from compute_rates to run method, so that compute_rates would be called inside run, but also could be used apart from it. This doesn't change anything for the outside users/methods (snowglobes.simulate will work absolutely the same)

OK, I got what you suggest now. I don't see any issues with that, let's go for it. Also dropping the underscore works.

Sheshuk commented 1 year ago

Thank you for the feedback! I'll try to be more precise in the descriptions in the future.