-
Notifications
You must be signed in to change notification settings - Fork 27
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
Refactor of Fitting Infrastructure #81
Comments
I like the general setup of this. @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. |
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 😆 |
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 grid3 main objects:
What's NeededFitter
Model
from astropy.modeling import Fittable1DModel, Parameter
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 above is my suggested approach based on my current thinking. Open to changes as we discover more about implementing this in practice. |
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.
This should allow us to push ahead with the foundational infrastructure and deal with the "nice" user interface later. |
Is very specific I would say need to abstract away to a |
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 |
I guess the main point is that our 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. |
I have seen the forward method in your toy-fitter code, what exactly is the purpose of the forward/apply methods? |
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 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 |
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
More text to follow...
The text was updated successfully, but these errors were encountered: