Skip to content
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

Make the Detector and flux.Container classes a public interface for v2.0 #339

Open
Sheshuk opened this issue May 30, 2024 · 2 comments
Open
Labels
enhancement New feature or request suggestion An idea that needs to be discussed/approved before starting implementaion

Comments

@Sheshuk
Copy link
Contributor

Sheshuk commented May 30, 2024

Some time ago I introduced the object-oriented interfaces for manipulating

They are documented, though the documentation might need an update and improvement.
For Detector, DetectionChannel and SmearingMatrix the docstrings are in place (see code) but they are not shown in the Sphinx docs yet.

They also have the extensive usage example notebooks: Detector_demo.ipynb, FluxContainer_demo.ipynb

These interfaces have always been marked with "Users should not use this" sign. The plan was to test them in a development team, but I don't know if anyone was using them (at least I didn't get any feedback or suggestions).

I use snewpy for the rate calculations for various detectors - not only the ones defined in the SNOwGLoBES, and as a user I find the standard interfaces with generate_* extremely restricting and unflexible and incapable of doing what I need.

So I suggest to

  1. Declare this interface public in v2.0
  2. Update the documentation - make the classes visible
  3. Add usage examples to the main section of the documentation
@Sheshuk Sheshuk added enhancement New feature or request suggestion An idea that needs to be discussed/approved before starting implementaion labels May 30, 2024
@JostMigenda
Copy link
Member

A couple of points off the top of my head, in no particular order:

  • I agree that generate_* (and, honestly, the other functions in snewpy.snowglobes, too) in SNEWPY v1.x are unflexible; and we’ve discussed changing this in v2.0. To what degree does that resolve your issues? If there are use cases that still don’t work, we should discuss if we could cover them as part of the snewpy.snowglobes redesign.

  • I think we’ve had a couple of PRs building upon those private APIs, right? And speaking at least for myself, my summer student and I have explored these APIs a bit, and have been quite happy with them. So I’d say the lack of feedback is an indication that these APIs have proven well-designed thus far—nice work! 👏

  • Do you think these APIs will need any non-trivial changes to fit in with all the other changes planned for v2.0?

  • Is there any particular reason this would need to be in v2.0 exactly, or would it be possible to leave this e.g. for v2.1? (The reason I’m asking is, v1.5 was already a bit more painful than I’d like a release to be; and I worry that v2.0 is going to be so hugely complex, that even with one or two beta versions, it’ll be a lot of work. Especially if we want to hurry to present v2.0 at the summer conferences and get it released soon thereafter, I’d rather not expand the scope of that release significantly further.)

  • Of the new detectors you’re defining—are there any that are suitable and ready for general usage? If so, could you open a PR to add the(se) detector(s) to SNOwGLoBES?

@Sheshuk
Copy link
Contributor Author

Sheshuk commented Jun 6, 2024

and we’ve discussed changing this in v2.0. To what degree does that resolve your issues ? If there are use cases that still don’t work, we should discuss if we could cover them as part of the snewpy.snowglobes redesign.

Are you referring to #209?
No, I don't think any update of these functions will be sufficient.
The problem with the generate_* functions is that they pack too many steps in one. - even if they're updated, it still means that it does:

  1. Initialize a model
  2. Get the neutrino flux from the model
  3. Apply flavor transformation
  4. Integrate the flux in the required time&energy bins
  5. Select the detector from SNOwGLoBES data
  6. Calculate the number of events in this detector

And it does all of this under the hood. That means the user has no access to the neutrino flux, or flavor transformation, or the detector setup.
There are many use cases, when user needs to examine and/or modify them - in between these steps.

I understand that having one function is intended to keep the interface simpler for the user. But it serves only a single use case: if the user needs neutrino interactions in the detector.
Other use cases, like plotting the neutrino flux and getting the detector information requires a user to use other functions (SupernovaModel.get_flux, or examining the snowglobes_data), and these tasks are very common.

I think we’ve had a couple of PRs building upon those private APIs, right? And speaking at least for myself, my summer student and I have explored these APIs a bit, and have been quite happy with them

Thanks, that's nice to know! So they might be useful for others as well

Do you think these APIs will need any non-trivial changes to fit in with all the other changes planned for v2.0?

I think only these:

In other aspects these classes will remain the same (if I'm not missing anything).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request suggestion An idea that needs to be discussed/approved before starting implementaion
Projects
None yet
Development

No branches or pull requests

2 participants