Skip to content

Adds fitter#258

Open
jajmitchell wants to merge 7 commits intosunpy:mainfrom
jajmitchell:adds-fitter
Open

Adds fitter#258
jajmitchell wants to merge 7 commits intosunpy:mainfrom
jajmitchell:adds-fitter

Conversation

@jajmitchell
Copy link
Copy Markdown
Contributor

Adds the fitter object

This PR adds the fitter object which intakes the NDcube spectrum object and can perform fitting in count space. This includes the addition of a plotter for fit results.

Tasks:

  • Generalise the plotting code
  • Tidy up and generalise the unit handling in instrument_response.py
  • Add an instance saving method to the fitter
  • Add examples

Copy link
Copy Markdown
Member

@samaloney samaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, nice to see it finally working!

The biggest things I see missing are docstrings and test, as this is a core part of the package we need to though testing e.g hight level testfit non-spectral data/model, fit spectral data model with out SRM, fits spectral data model with SRM and also more basic unit tests.

On a similar topic an gallery example showing a some of the basic fits you can and some narrarive documentation on the fitter/fitting (this could wait for a separate PR)

Comment on lines +45 to +55
def _set_abledo_angle(self):

if 'Albedo' in self.model.submodel_names:

print(len(self._spectrum_object.meta['ph_axis']))

replacement_albedo = Albedo(energy_edges=self._spectrum_object.meta['ph_axis'],
theta=self._spectrum_object.meta['angle'])
replacement_albedo.theta.fixed = True

self._model = self._model.replace_submodel('Albedo',replacement_albedo)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be derived from the Spectrum object e.g. spec.observer_location and spec.flare_location which would be SkyCoords


self._model = self._model.replace_submodel('Albedo',replacement_albedo)

def _set_observer_distance(self):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derived from spec.observer_location

Comment thread sunkit_spex/fitting/fitter.py Outdated

mask = self._fit_mask

self._spectrum_object = self._spectrum_object[mask[0]:mask[-1]+1]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using the mask / or subscripting the final data before it goes into the fitter? Also what about the SRM?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SRM is clipped just below this

Comment on lines +18 to +19
data_spec_units=u.dimensionless_unscaled,
conversion_factor=1*u.dimensionless_unscaled,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually these should probably live the the SRM object (an NDCude with 2 spectral axes in input or photon and one output, count or data)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't show up here needs to merge or rabase main

Comment thread sunkit_spex/visualisation/plotter.py Outdated
@jajmitchell jajmitchell mentioned this pull request Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants