Open JostMigenda opened 2 years ago
@JostMigenda, how about we just pass default parameters (theta=0
, phi=0
)?
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
.
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.
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.
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.
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?
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.
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)?
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:
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.
The
Fornax_2019
model’sget_initial_spectra()
method has two required arguments not used by other models:theta
andphi
of the observer direction. Sincegenerate_time_series()
/generate_fluence()
doesn’t include those arguments when it callsget_transformed_spectra()
, which calls the respective model class’sget_initial_spectra()
, code like(adapted from
SNOwGLoBES_usage.ipynb
) will result inTypeError: 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__()
andget_initial_spectra()
have additional arguments with the same name.