Make signature for `SupernovaModel.get_*_spectra` uniform for all models/subclasses

[Sheshuk]

What we have now

The signature signatures for these functions in the base SupernovaModel class are: https://github.com/SNEWS2/snewpy/blob/b35379952fe18aedc28cdb106f1ee9e7ad08b082/python/snewpy/models/base.py#L99 https://github.com/SNEWS2/snewpy/blob/b35379952fe18aedc28cdb106f1ee9e7ad08b082/python/snewpy/models/base.py#L126

Currently most of the models conform to the base signatures, but some models require additional parameters, like Fornax*: https://github.com/SNEWS2/snewpy/blob/b35379952fe18aedc28cdb106f1ee9e7ad08b082/python/snewpy/models/ccsn.py#L664

Why is it bad

Having different signatures in base and children classes might lead to confusion and errors. In the Fornax2019 case, for example, there is no way to pass the additional parameters to the get_transformed_spectra, so running the following code:

from snewpy.models import ccsn
from snewpy.flavor_transformation import NoTransformation

model = ccsn.Fornax_2019('models/Fornax_2019/lum_spec_10M.h5')
model.get_transformed_spectra(t,E, NoTransformation())

results in

TypeError: Fornax_2019.get_initial_spectra() missing 2 required positional arguments: 'theta' and 'phi'

What I suggest

Add the **kwargs argument to the signatures:

def get_initial_spectra(self, t, E, flavors=Flavor, **kwargs):
    ...
def get_transformed_spectra(self, t, E, flavor_xform, **kwargs):
    ...

Optional (subjective opinion)

Method get_initial_spectra has flavors argument, which is not present in get_transformed_spectra. This also adds some confusion - as a user I would expect that these methods have the same arguments, apart from the flavor_xform. I see no need to pass flavors to get_initial_spectra - if a user needs only specific flavors, he can just filter what he needs from the output.

So I would even go for

def get_initial_spectra(self, t, E, **kwargs)

[JostMigenda]

For Fornax_2019, the issue of theta and phi was already raised in #165. (Though I didn’t get to implement the discussed fix during the hackathon and lost track of it afterwards; thanks for the reminder!) I’m not sure about the extra interpolation="linear" argument (which exists in the Fornax_2021 subclass, too); does the other value ("nearest") actually have real use cases? As far as I can tell, no other model subclasses are affected, right?

Regarding the “optional” section: It is true that a user can filter the flavours later; but calculating fluxes for unused flavours can take a significant amount of time. (E.g. I added this as an optional argument in #50 because it cuts sntools’ total run time by about ~40% when using snewpy models.) Since there is a sensible default (all flavours), this extra flavors argument can be easily ignored and will not cause TypeErrors. I guess we could add the same argument to get_transformed_spectra for consistency; but in sntools at least, I don’t currently see a need for it.

[Sheshuk]

For Fornax_2019, the issue of theta and phi was already raised in #165

Thanks for pointing to these issues, I somehow missed them. I have a more general question then: even if we manage to remove all the additional arguments in Fornax, are we sure that new models in the future will come with no additional arguments?

Having **kwargs would help us to avoid "adapting" the new models signatures by removing these optional arguments like interpolation

I’m not sure about the extra interpolation="linear" argument (which exists in the Fornax_2021 subclass, too); does the other value ("nearest") actually have real use cases?

I would leave that question to the models authors/implementers. If they provide such an option, SNEWPY interface should be capable of using it.

[Sheshuk]

calculating fluxes for unused flavours can take a significant amount of time

There are other means of improving the performance, in particular using the vectorized calculations instead of python loops (for example #221). As I showed in my previous take on this in #152 , this will speed up our calculation at least ~5 times, so this 40% benefit might not be that dramatic now?

[JostMigenda]

There are other means of improving the performance […]

Those speedups are certainly useful, but could easily be canceled out if we go from 4 to 6 flavours in a future version, add a model with finer time binning, etc. Obviously, if the performance benefit becomes negligible, we don’t need that extra argument anymore. But since it’s completely optional, with a sensible default, and adds no extra code in the function body, the downside truly is minimal and I think it’s well worth having, even if the upside becomes smaller.

are we sure that new models in the future will come with no additional arguments?

No. But even if they do, I’m not sure how adding **kwargs to the function signatures in the base class would help? I guess for the interpolation argument, it is possible to just pass that through from get_transformed_spectra to get_initial_spectra, so that the Fornax class doesn’t have to overwrite get_transformed_spectra; but for other arguments, passing them through might not work.

[Sheshuk]

I’m not sure how adding **kwargs to the function signatures in the base class would help?

It makes our base class interface (methods signature) consistent with the children classes. Python is not strict with the abstract/derived classes methods signature, so you can have what we have in Fornax now - the get_initial_spectra can be redefined to whatever arguments, but that's a bad thing.

If in the base class you define some interface it is a convention, a protocol all the children classes must follow. Having it otherwise makes a lot of confusion and removes any sense of deriving from the base class - and that's what we have now.

I guess for the interpolation argument, it is possible to just pass that through from get_transformed_spectra to get_initial_spectra, so that the Fornax class doesn’t have to overwrite get_transformed_spectra; but for other arguments, passing them through might not work.

Exactly as you say:

class SupernovaModel:
    ...
    def get_initial_spectra(self, t, E, **kwargs):
        ...
    def get_transformed_spectra(self, t, E, flavor_xform, **kwargs): 
        spectra = self.get_initial_spectra(t, E, **kwargs) #passing all the extra arguments here

class ComplicatedModel:
    ...
    def get_initial_spectra(self, t, E, *, important_parameter, other_parameter):
        ...

Then calling ComplicatedModel.get_transformed_spectra(..., important_parameter=1, other_parameter=0) will work as it should.

I don't see why would that not work any arguments, other then interpolation?

[JostMigenda]

Python not being strict about this is, in my eyes, a feature; not a bug. I agree that this flexibility can, in principle, be abused to write confusing code; but in this case, I see very little risk of any developer being confused by that additional interpolation argument. (Though, to be fair, this is such a minor question of preference that I wouldn’t be bothered by this.)

However, I have a slightly more serious issue with this as well: Adding **kwargs to the function signature in the base class signals to people adding new models that it’s fine (or even recommended) to extend the function signature in the base class; and right now, it very certainly isn’t, because it breaks things. I would also argue that we shouldn’t move in that direction, because it would require to pass those extra arguments around (e.g. if users use snewpy.snowglobes functions), and that adds complexity and may potentially fail in some edge cases.

I don't see why would that not work any arguments, other then interpolation?

For example, it wouldn’t work for the flavor argument (except with flavor_xform=NoTransformation, of course): Let’s say we’re only interested in inverse beta decay, i.e. in the transformed spectra of NU_E_BAR. But to calculate that for e.g. AdiabaticMSW, we’d need the initial spectra of NU_E_BAR and NU_X_BAR (and potentially other ones, for more exotic flavour transformations); so just passing the flavor argument through would raise an error.