sunpy / sunkit-spex

:construction: Under Development :construction: A package for solar X-ray spectroscopy
BSD 3-Clause "New" or "Revised" License
22 stars 26 forks source link

Refactor of Fitting Infrastructure #81

Open DanRyanIrish opened 2 years ago

DanRyanIrish commented 2 years ago

Provide a general description of the issue or problem.

The fitting infrastructure provided by @KriSun95 is a valuable addition to the package. To help modularity, flexibility and ease of maintenance, a refactor would be beneficial. This issue sketches out how this could be done.

Components

Data Structure(s) Requirements

Spectral Response Model

Spectral Response Model Collection

settwi commented 11 months ago

I like the general setup of this. I think we should avoid inheritance unless it will offer an obvious benefit to the code. We can just use composition (i.e. a "has-a" NDCube rather than an "is-a" relationship). It makes it easier to swap things out later.

@KriSun95 have been thinking of having a mini-workshop for a few days at some point in the near future to hash out some ideas too.

samaloney commented 11 months ago

I think the main thing is to define the API. How that API is implemented e.g composition, inheritance or a mix is a detail, all be it an important one. Ideally we would have something something like the NDCube SEP realistically probably a simlified version 😆

DanRyanIrish commented 10 months ago

After the discussion at the STIX meeting last week, here's an update on one option for the refactor

Overall structure for fitting a single model to a spectrum/multiple spectra on same spectral grid

3 main objects:

What's Needed

Fitter

Model

class MatrixModel(Fittable1DModel): matrix = Parameter(description="The matrix with which to multiply the input.", fixed=True)

@staticmethod
def evaluate(x, matrix):
    return matrix @ x  # Requires input must have a specific dimensionality

* The input of the total model needs to be fixed to the spectral coords of the photon axis of the DRM.  This can be done with the ``astropy.modeling.fix_inputs` function.
* Physcial models in `sunkit-spex` needs to be converted to astropy models, or at least functions which can be wrapped in the `custom_model` decorator.
  * I am thinking in particular of the `thermal_emission` model which is currently a class.  But this should be fixable.

### `Spectrum` object
* Uncertainty objects need to be written in line with the `astropy.nddata.NDUncertainty` API.  These are needed for the following uncertainty types:
  * Poisson
  * etc.
* The fitter can check the type of this on the `Spectrum` object and use the correct error handling.
  * Perhaps some of the error handling methods can be attached directly onto these objects, similar to `StdDevUncertainty` and `VarianceUncertainty`.
  * A long term goal could be to upstream these to astropy, if we can make them follow the same API.

# Simultaneously fitting different models to multiple spectra OR same model to spectra on different spectral grids
* The `Fitter` should accept `tuples` of `(Spectrum, Model)` pairs. The fitting is then done by minimised the sum of the log-likelihoods of the different models to the different spectra.
Not sure whether this needs to be a separate collection fitter class, or if the same `Fitter` object should handle this case and the above one.
DanRyanIrish commented 10 months ago

The above is my suggested approach based on my current thinking. Open to changes as we discover more about implementing this in practice.

DanRyanIrish commented 9 months ago

After our productive discussion yesterday, I thought it would be helpful to clarify what I took from others' contributions, and how my thoughts have evolved on reflection. The main concerns I remember being raised were:

My feeling is that the reason we are finding these discussions so difficult, is that we are not sufficiently separating and abstracting the different functionalities we need and their requirements. Therefore, we often get trapped in discussions about a whole complex assortment of functionalities that the fitter should handle.

To make progress, I suggest that we focus only on the fundamentals of what we need for fitting, namely:

This is the smallest set of infrastructure that enables users to do fitting.

If we want to enable a more user-friendly system that restricts chances for user error, I suggest we assign this to a future "fitting environment class", to be defined in the future. This could potentially take

This object then support data/srm manipulation so long as the spectrum and srm support the same operations, e.g. .rebin(). Then, when it comes time to fit, a new fitter instance is generated with the self.spectrum object(s) and an on-the-fly-generated the model: model = self.pre_srm_model | self.srm | self.post_srm_model e.g. self.generate_fitter(*args) which doesn't something like self.fitter = self.fitter_type(self.spectrum, self.pre_srm_model | self.srm | self.post_srm_model) This could be done in a way in which various settings, e.g. which parameters are fixed, tied, etc., persist. Then the user could do fitter_env.fitter.fit() The API could also support multiple spectrum/model pairs for simultaneous fitting. The structure and API of this "fitting environment class" can be haggled over at a later stage. But crucially, this abstraction depends on the foundational infrastructure of

This should allow us to push ahead with the foundational infrastructure and deal with the "nice" user interface later.

samaloney commented 9 months ago

A MatrixModel that follows the astropy modeling API and can therefore act as a component in a compound model (including physical models, optical path models, etc.) This class acts as a wrapper around any object that supports the @ operator, i.e. matrix multiplication. This can be as simple as a numpy array, or a more complex instrument-specific SRM object with any number of methods, e.g. rebin.

Is very specific I would say need to abstract away to a Response that has some method forward, apply which is called or something e.g. no only / specifically matrix multiply

DanRyanIrish commented 9 months ago

I agree. There is probably a minimal API required by a representation of a response, e.g. the input and output spectral grid. But such a response object must be able to be a component in a compound astropy model, or be able to be wrapped in something that enables that.

My MatrixModel suggestion is simply a demonstration of how a response object could be wrapped into an astropy model component in the specific case where the response is represented by a matrix. But the fitting infrastructure should, in principle, support any astropy model.

samaloney commented 9 months ago

I guess the main point is that our Response object is more like a Spectrum object in terms of interface and function than a model. Also a large fraction of the time it's fixed/static so why are we trying to force into a model when are already have 90% of the required API and some code from the Spectrum/NDCube object?

We can always support wrapping a Response object with a model decorator if it needs to be varied in the fitting process but that not required right now to make progress.

natsat0919 commented 9 months ago

A MatrixModel that follows the astropy modeling API and can therefore act as a component in a compound model (including physical models, optical path models, etc.) This class acts as a wrapper around any object that supports the @ operator, i.e. matrix multiplication. This can be as simple as a numpy array, or a more complex instrument-specific SRM object with any number of methods, e.g. rebin.

Is very specific I would say need to abstract away to a Response that has some method forward, apply which is called or something e.g. no only / specifically matrix multiply

I have seen the forward method in your toy-fitter code, what exactly is the purpose of the forward/apply methods?

DanRyanIrish commented 9 months ago

I guess the main point is that our Response object is more like a Spectrum object in terms of interface and function than a model. Also a large fraction of the time it's fixed/static so why are we trying to force into a model when are already have 90% of the required API and some code from the Spectrum/NDCube object?

I think this is an implementation detail. The epiphany I had after Wrocław is that philosophically, the response IS just a model component like any other, e.g. the thermal_emission model. It is a step in a chain of calculations that goes from photons emitted at the Sun, to counts measured by the detector. The fact that it may (or may not) be represented underneath as a matrix is a technicality, so long as it is possible to wrap the underlying matrix representation as a model, which the MatrixModel class proves it can be. This massively simplifies our design philosophy to 3 objects, i.e. spectrum + model + fitter.

Of course, we know that in practice, most users will want to use a response represented by a matrix. So we can provide convenience features for them to convert their matrix into a model, i.e. the MatrixModel class or an equivalent. We also know users may want to link the spectrum and response so that both are altered consistently, or forego the hassle of manually building the whole model themselves. Therefore, we can provide a sandbox in which these things can be done with reduced chance of user error and effort. But these are all convenience features built on top of a simpler, more powerful, albeit more error-prone, 3-object foundational API of spectrum + model + Fitter.