More generic API for Rainbow variants

[karpov-sv]

After discussion with @erusseil we decided to unify the zoo of Rainbow variants.
So the PR replaces them all with a single RainbowFit feature that, on initialization, accepts keywords bolometric and temperature that may be either strings (for built-in variants) or custom sub-classes of corresponding base classes for bolometric and temperature terms. Default values should match the behaviour of previous version of RainbowFit.

The built-in variants for bolometric term are:

• bazin - symmetric form of Bazin function, normalized to unity to de-correlate the amplitude from timescales
• sigmoid - rising sigmoid

Temperature options are:

• sigmoid - falling or rising sigmoid, with reference time equal to the one of bolometric term
• constant - constant temperature
• delayed_sigmoid - again sigmoid, but with the delay between temperature and bolometric reference times as a free parameter

The PR also slightly improves initial guesses for Bazin parameters, and fixes minor bugs from previous code versions.
Also, it changes the normalization of the underlying Planck term to be more straightforward - it both de-clutters the code a bit (basically, by moving the normalization directy to the term from extra scaling/unscaling methods), and returns the amplitude parameter in (approximately) the same units as input data, which should be more understandable by the end user.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.94%. Comparing base (84f276f) to head (267cf92).

❗ Current head 267cf92 differs from pull request most recent head ee90dfb. Consider uploading reports for the commit ee90dfb to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #324   +/-   ##
=======================================
  Coverage   84.94%   84.94%           
=======================================
  Files           9        9           
  Lines        2664     2664           
=======================================
  Hits         2263     2263           
  Misses        401      401           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codspeed-hq]

CodSpeed Performance Report

Merging #324 will not alter performance

_{Comparing karpov-sv:rainbow-generic (ee90dfb) with master (84f276f)}

Summary

✅ 97 untouched benchmarks

[hombit]

Thank you, @karpov-sv, it looks really great. I'll go through it a little later, sorry in advance for the delay

[hombit]

It is a huge and great contribution! Thank you so much for it.

Please find some discussion topics bellow.

Interface of RainbowFit

I believe that bolometric and temperature arguments of RainbowFit should be objects, not classes. I would remove @staticmethod from abstract methods and allow users to create parametric subclasses of BaseBolometricTerm and BaseTemperatureTerm. This interface would make string literals to correspond to the "default" objects of the built-in classes (see the discussion bellow).

bolometric.py and temperature.py

1. I'm not sure how idiomatic the inheritance approach used there: we go from variable length of parameters *params to fixed number of params. It should be fine because we are the only users of the child classes, and we hopefully always provide the parameters in a right order.
2. As I mentioned above, I would appreciate if we make child classes parametric. We can do it with @dataclass(frozen=True). Limits and initial guesses could easily be parameters, having the same default values as you currently use in the code. In this case we can use trivial constructors to create "default" objects for bolometric_terms and temperature_terms.

Scaling literals

I like the idea of specifying scaling rules in bolometric/temperature classes. I would ask to make the way we specify these rules more clear: add all variants to the base classes docs, verify them more strictly on the runtime.

[hombit]

@karpov-sv @erusseil sorry, I've forgotten to write about the amplitude. I see your point about amplitude normalization, but I don't like that the output value doesn't have a clear physical meaning. What about a compromise solution: could we output the final "amplitude" to be a peak flux value in the first passband? We still may have the same "amplitude" definition inside the code, but replace it with some meaningful value in the end.

[karpov-sv]

So I implemented most of your suggestions. For the rest I suggest to keep it as it is now.

Parametric subclasses would not work for the current approach - at least if you wish to pass initial guesses as the parameters to them. These classes operate on scaled variables, and have no way to access the original scalers - thus, their initial parameters are not related to anything the end user may provide, and cannot be converted. (It would also break the concept of "batch processing", no?..).
I believe the proper place to introduce initial-guesses-from-outside is a non-batch interface like fit_and_get_errors() where you do actually have both scalers and scaling rules for the parameters. This way it will also keep the feature pure.

As for the amplitude in the parameters - now it is physically in the same (roughly) scale as original data. It all goes down to the normalization of Planck function, which is still an open question. E.g. one idea is to actually normalize it to the flux at some reference wavelength - it would break the meaning of the first term as bolometric, but would allow better fitting supernovae. However, it cannot be just one more extra parameter for the fit - the fitter is way too unstable as it is now already...
Let's keep it as it is now, and discuss it next week in Clermont.

[hombit]

@karpov-sv sounds great! I agree about the parametric subclasses, but I still want us to find a better solution for the amplitude.

[karpov-sv]

It still tells me that changes are requested, but I see no more open questions here. What should I do?..

[hombit]

@karpov-sv would you like me to merge it now, so we can work on the next PR in Clermont?

[karpov-sv]

Yes, let's merge it as it is now, and discuss what to do next in Clermont.

[hombit]

@karpov-sv I'll do it! Sorry for the mis-communication