sunkit-spex icon indicating copy to clipboard operation
sunkit-spex copied to clipboard

Astropy model fitting

Open KriSun95 opened this issue 1 year ago • 2 comments

This PR aims to start introducing code in the fitting refactor, first aiming to implement astropy models (discussion started in #81).

A few scripts are included into the fitting module and split into sub-modules. These sub-modules revolve around the models being used to fit data, general objective functions, somewhere for different statistics, and one for optimiser tools.

The thought process behind the modules is:

  • models: Somewhere to keep all the cool models we want.
  • statistics: The functions in here should only take in the data and the model arrays that need to be compared (errors eventually, etc). This module could have a bunch of different statistics (e.g., Gaussian, Poissonian, etc.).
  • objective_functions: these hold the functions that are actually being minimised. they take in the information to evaluate a given model at a bunch of different parameters and pass the model result over to whatever fit statistic you want.
  • optimizer_tools(should be optimiser_tools really!): Handles the interface between the objective function and whatever fitting routine is used. In the case here, script.minimize.

An example folder is created that includes a very simple example script showing the generation of fake data to be fitted, a dummy instrument response matrix, and the fitting of that fake data using scipy and astropy. The results are also plotted in the script and the result should be the following astropy-fitting-example

Feel free to play around with the code and any feedback is encouraged!

Next steps could include, but are not limited to

  • dealing with non-square instrument responses
  • simultaneous fitting

Note: An additional change was implemented in sunkit_spex/legacy/fitting_legacy/fitter.py as numpy has moved their warnings to another model in v2, I think.

KriSun95 avatar Jun 17 '24 22:06 KriSun95

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Please upload report for BASE (main@3a9a4a4). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #155   +/-   ##
=======================================
  Coverage        ?   50.42%           
=======================================
  Files           ?       30           
  Lines           ?     3443           
  Branches        ?        0           
=======================================
  Hits            ?     1736           
  Misses          ?     1707           
  Partials        ?        0           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jun 27 '24 20:06 codecov-commenter

Quite a few things have been updated. I focussed on addressing the suggestions made by @samaloney which has put the code in a much better spot, I think. I've also added in a number of tests for the stuff I wrote as well.

KriSun95 avatar Jun 28 '24 22:06 KriSun95

@hayesla @DanRyanIrish could one of you review so we can maybe get this merged

samaloney avatar Sep 10 '24 15:09 samaloney

I don't know where the best place to discuss this is, but I have been working on improving the performance of astropy modelling fitters and implemented a helper function for parallel fitting I would be super interested to hear your thoughts on what would need to be done to make this useful for your use cases.

Cadair avatar Sep 12 '24 11:09 Cadair

I think the StraightLineModel and GaussianModel seem like placeholder classes. Unless they gain more functionality than the astropy versions, they should be removed in the future. But I don't want to hold this up getting merged.

DanRyanIrish avatar Sep 19 '24 14:09 DanRyanIrish