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

Astropy model fitting #155

Closed KriSun95 closed 1 month ago

KriSun95 commented 4 months ago

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:

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

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.

codecov-commenter commented 4 months ago

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 ```diff @@ 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.

KriSun95 commented 4 months ago

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.

samaloney commented 1 month ago

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

Cadair commented 1 month ago

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.

DanRyanIrish commented 1 month ago

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.