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

Fornax_2019 model cannot be used with snewpy.snowglobes #165

Open
JostMigenda opened this issue Dec 14, 2021 · 9 comments
Open

Fornax_2019 model cannot be used with snewpy.snowglobes #165

JostMigenda opened this issue Dec 14, 2021 · 9 comments
Assignees
Labels
bug Something isn't working SNOwGLoBES Related to SNOwGLoBES SupernovaModel Implementing/correcting supernova model

Comments

@JostMigenda
Copy link
Member

The Fornax_2019 model’s get_initial_spectra() method has two required arguments not used by other models: theta and phi of the observer direction. Since generate_time_series()/generate_fluence() doesn’t include those arguments when it calls get_transformed_spectra(), which calls the respective model class’s get_initial_spectra(), code like

modeltype = 'Fornax_2019'
model = 'lum_spec_15M.h5'
transformation = 'NoTransformation'
distance = 10
outfile = f"{modeltype}_{model}_{transformation}"
tarredfile = snowglobes.generate_fluence(f'models/{model_type}/{model}', modeltype, transformation, distance, outfile)

(adapted from SNOwGLoBES_usage.ipynb) will result in TypeError: get_initial_spectra() missing 2 required positional arguments: 'theta' and 'phi'.

Similar to what @soso128 suggests in #164, this could be fixed by passing arbitrary **kwargs along that call chain; though we should be careful to avoid edge cases like if a model class’s __init__() and get_initial_spectra() have additional arguments with the same name.

@JostMigenda JostMigenda added bug Something isn't working SNOwGLoBES Related to SNOwGLoBES SupernovaModel Implementing/correcting supernova model labels Dec 14, 2021
@sybenzvi
Copy link
Contributor

@JostMigenda, how about we just pass default parameters (theta=0, phi=0)?

@JostMigenda
Copy link
Member Author

That would fix this error only superficially; the bigger issue of not being able to set it would remain.

I guess it depends: Is there a good default direction that will cover most use cases of this model? Or is a major reason to make the extra effort to include full directional dependence of the signal that we want to let users compare multiple direction? If it's the latter, then we should try to expose that in snewpy.snowglobes.

@sybenzvi
Copy link
Contributor

sybenzvi commented Dec 15, 2021

In principle the direction is interesting: see the Fornax 2019 notebook for how much the anisotropy changes as the explosion evolves. And I expect there will be future models that allow for anisotropic emission, so it's worth finding some solution. Passing kwargs may be the right approach, since the alternative would seem to be requiring these parameters get introduced in the base class... which is ROOT's approach to object-oriented programming. I don't think that's acceptable.

If this is actually causing a test failure, we could temporarily use the default settings, close this as a bug fix, and make a lower-priority enhancement ticket to enable anisotropic emission models. It's up to you and your opinion of how serious the issue is at the moment.

@JostMigenda
Copy link
Member Author

It’s not causing a test failure, since we’re not testing that model with SNOwGLoBES currently; so no need for a temporary fix.

Additional note: See the changes to Fornax_2021 in #166—once those are reviewed, we’ll need to apply similar changes here, too.

@JostMigenda
Copy link
Member Author

JostMigenda commented Jul 28, 2022

At the SNEWPY developer/user meeting today, we discussed that it may be better to move these arguments into the initialiser. That way the existing kwarg mechanism would work without any changes to the rest of the SNEWPY code; and the model registry code which constructs a file name from the physics parameters can simply ignore theta and phi.
I’ll assign myself and try to do that during the hackathon.

@Sheshuk
Copy link
Contributor

Sheshuk commented Oct 13, 2022

I missed this discussion before.

it may be better to move these arguments into the initialiser

Do I understand correctly that if a user wants to plot this model's predictions for N (theta, phi) values (say, scan all possible values grid), he will need to initialize the model N times, reading the same files over and over again, just to calculate the values for new theta, phi pair?

@JostMigenda
Copy link
Member Author

In principle yes.
In practice, we could simply write those values to self.theta and self.phi during initialisation and then use those in get_initial_spectra instead of passing them in as arguments.
That way, if the performance overhead of initialising the model multiple times is significant, we could document that users can initialise it from the file once and then change those instance variables as needed.

@Sheshuk
Copy link
Contributor

Sheshuk commented Oct 14, 2022

You propose a model to maintain some state (keeping the angles) only to use it when the get_initial_spectra is called - and make user to modify this state.
How is this better in any way, then keeping these parameters in the get_initial_spectra, (where they actually belong)?

@JostMigenda
Copy link
Member Author

I agree that that part is not elegant. However, it does make enough other parts of snewpy better, that I think it’s worth the trade-off:

  • Now that we are moving towards initialising a model from physics parameters (instead of the file name) by default, I think it’s cleaner to initialise with all physics parameters (including observer direction) up front; rather than initialising with some PPs and then giving additional PPs in some later function calls.
  • Note also that for other 3D models, it is not always possible to set the observer direction later on—for example, the Garching models typically have separate input files for each observer direction. And setting the same parameters in different places for different models, depending on implementation details that the user shouldn’t have to worry about, is not a great design either.
  • Separately, we’ve already discussed that we want to move to a design of snewpy.snowglobes where the user initialises a SN model first and then passes it to the generate_* function (instead of passing a string and having the initialisation happen inside the generate_* function). That means that it’s easy to deal with additional parameters in initialisation; but additional parameters in get_initial_spectra require extra complexity to pass them around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working SNOwGLoBES Related to SNOwGLoBES SupernovaModel Implementing/correcting supernova model
Projects
None yet
Development

No branches or pull requests

3 participants