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

Improve generate_* functions in snewpy.snowglobes #209

Open
4 tasks
JostMigenda opened this issue Sep 15, 2022 · 4 comments
Open
4 tasks

Improve generate_* functions in snewpy.snowglobes #209

JostMigenda opened this issue Sep 15, 2022 · 4 comments
Assignees
Milestone

Comments

@JostMigenda
Copy link
Member

There are a couple of problems with the generate_fluence and generate_time_series functions in snewpy.snowglobes:

  • The difference between both functions is subtle and not well documented currently.
  • The arguments are inconsistent for no obvious reason: generate_time_series takes a number of bins or a bin duration, whereas generate_fluence takes tstart and tend which can be either a single value (corresponding to 1 bin) or a list of times (to create multiple bins of arbitrary lengths). As we recently saw on Slack, this even trips up us main developers sometimes.
  • Currently, the generate functions only support initialising a model from a file path¹. To switch to initialisation from physics parameters (see Model registry jan22 #184), we will need make some changes.
  • Currently, the flavour transformation is initialised from a string. This is already a problem today, because it means we cannot use e.g. transformations involving sterile neutrinos with user-specified mixing parameters; and it also will not be able to support composite transformations (First pass at composite transformations. #203) or different versions of mixing parameters (add MixingParameters presets #198) in the future.

These issues are in principle independent; but they may all require backwards-incompatible changes to resolve, so I think we should try to tackle them in a coordinated fashion to minimise disruption. I have a rough idea how to do that, but want to coordinate with people working on the PRs mentioned above first.

¹ There is an optional snmodel_dict argument, which we could use to fit in all the physics parameters; but that would be quite awkward to use (and we’d still have the (then unused) file path as a required argument).

@JostMigenda
Copy link
Member Author

We discussed this at the SNEWS telecon just now. A quick summary:

  • For SupernovaModel and FlavorTransformation, there was general agreement that those should be initialised by the user and then passed as objects rather than strings.
    • In some places where we need a stringified version of the model/transformation name (e.g. in filenames), the __str__ method is probably enough; or we may need to add a short method for that.
  • There was brief discussion about combining both functions into one, or just keeping the generate_fluence function, which is physically more accurate. Longer computation time was an issue in the past; but the implementation has changed somewhat since, so we should double check if that is still a concern.

@jpkneller
Copy link
Contributor

I like Andrey's suggestion for replacing the model_type and the transformation_type strings with class instances. With the current version of SNEWPY, flavor transformations which require additional parameters beyond the three vacuum mixing angles cannot be used unless the user dives into the generate_* functions. The two strings are included in the output filenames made by generate_* so we could modify both the flavor transformation and model classes by adding to each a to_tex() method which returns a string with the model or flavor transformation name. This is what is done in the Flavor class.
I would also like to suggest we deprecate the generate_time_series function and move to only having a generate_fluence function. For the time being, generate_time_series can be modified so that it creates the list of tstart and tend that then gets passed to generate_fluence.

@JostMigenda JostMigenda added this to the v1.4 milestone Sep 15, 2022
@mcolomermolla
Copy link
Collaborator

Commenting on the second point:
I am in favor of keeping only one function to make users life easier, and use generate_fluence.
Indeed, I remember it was taking long when using long duration and small binning simulations, but with the better performance after the updates and most of the models not being long, it is affordable. I can provide a computational time if you're interested. It is also true it is more accurate for simulating a lightcurve with "fine" binning.

@Sheshuk
Copy link
Contributor

Sheshuk commented Nov 24, 2023

I think this should be planned for SNEWPY v2.0, so I'll change the milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants