Pass SpectralAxis to physical models rather than energy edges by jajmitchell · Pull Request #223 · sunpy/sunkit-spex

jajmitchell · 2025-07-08T16:42:00Z

This PR enables the physical models to be used with a SpectralAxis

By passing a SpectralAxis to the physical models, they are effectively evaluated at bin centers, but have access to the bin edges as these are stored as an attribute in the SpectralAxis class. This change will allow for a clearer path to using Astropy model classes for fitting as the dimensionality of the input and output will no longer be an issue.

Adds SRM units and passes Spectralaxis to thermal

Checks input is SpectralAxis object, converts and warns if not

Adds check input function

ThickTarget and ThinTarget use centers

Albedo now takes SpectralAxis and centers

Pre-commit

scaling.py and models.py now function with centers

Fixes scaling and albedo tests

Adds __array_finalize__ method to SpectralAxis

Fixes tests

Fixes part of fitting_simulated_data.py

Reverts SRM model to main version

Fixes fitting tests

Fixes test_models.py

Fixes Albedo example

Fix Albedo example 2

Fixes examples

Adds changelog

Pre_commit fix

samaloney · 2025-07-21T10:50:28Z

Nice is this ready for review?

jajmitchell · 2025-07-21T14:30:11Z

Nice is this ready for review?

@samaloney I need to just check that the fitting works with the physical models, then I think its good to go

jajmitchell · 2025-07-22T13:54:55Z

@samaloney the complications continue. The issue comes when fitting, specifically in Astropy fitting.py line 255, as can be seen below.

if isinstance(x, Quantity): 
    add_back_units = True
    xdata = x.value
else:
    xdata = np.asarray(x)

This means that for fitting, before being passed to evaluate, the fitter takes the value of the SpectralAxis object, which not only strips the units, but also all other attributes. I'm trying to think of ways around this, but haven't come up with anything yet.

Adds custom `__call__` to `ThermalEmission`

Fixes test_thermal.py

jajmitchell · 2025-07-28T13:54:30Z

I have added a custom __call__ onto the ThermalEmission class, this is unnecessary for the ThickTarget as we evaluate at centers anyway

Adds meta to SpectralAxis, thermal model can now take Spectrum object, Spectral axis, or array

Can pass array without initialising

Fitting works

Tidy up

More tidy

Remove unused functions

Fixes continuum and line models

settwi

This is great, I think a few tweaks need to happen, the main two things i noticed:

rename variables from "e_centers" to spectral axis
enforce the SpectralAxis API everywhere; throw an error if SpectralAxis not used, don't try to coerce centers or edges into an axis. Keeps the API clear for users.

settwi · 2026-02-26T18:58:58Z

sunkit_spex/models/physical/tests/test_albedo.py

 def test_albedo_model():
    e_edges = np.linspace(10, 300, 10) * u.keV
-    e_centers = e_edges[0:-1] + (0.5 * np.diff(e_edges))
+    e_centers = SpectralAxis(e_edges, bin_specification="edges")


Variable name should change (here and elsewhere) to reflect the updated type

settwi · 2026-02-26T18:59:05Z

sunkit_spex/models/physical/tests/test_albedo.py

-    e_c = e_ph[:-1] + 0.5 * np.diff(e_ph)
-    spec_in = e_c**-2
+    e_ph = (np.arange(11) * 2 + 10) * u.keV
+    e_c = SpectralAxis(e_ph, bin_specification="edges")


Change variable name

settwi · 2026-02-26T18:59:54Z

sunkit_spex/models/physical/tests/test_thermal.py

    mg, al, si, s, ar, ca, fe = 8.15, 7.04, 8.1, 7.27, 6.58, 6.93, 8.1
    inputs = (
-        energy_edges, temperature, emission_measure, mg, al, si, s, ar, ca, fe, abundance_type
+        spectral_axis, temperature, emission_measure, mg, al, si, s, ar, ca, fe, abundance_type


might want this to be a dictionary?

settwi · 2026-02-26T19:00:20Z

sunkit_spex/models/physical/albedo.py

+        e_centers = SpectralAxis(np.linspace(5, 550, 600) * u.keV, bin_specification='edges')
        source = PowerLaw1D(amplitude=1*u.ph/(u.cm*u.s), x_0=5*u.keV, alpha=3)
-        albedo = Albedo(energy_edges=e_edges)
+        albedo = Albedo(spectral_axis=e_centers)


change variable name

settwi · 2026-02-26T19:00:42Z

sunkit_spex/models/physical/albedo.py

+def _check_input_type(spectral_axis):
+    if isinstance(spectral_axis, SpectralAxis):
+        energy_edges = spectral_axis.bin_edges
+    else:
+        warnings.warn(
+            "As a SpectralAxis object was not passed, bin edges will be calculated as averages from the centers given.",
+            UserWarning,
+        )
+        spectral_axis = SpectralAxis(spectral_axis, bin_specification="centers")
+        energy_edges = spectral_axis.bin_edges
+
+    return energy_edges


should throw an error if spectral axis not passed, not a warning. that's the API change right?

settwi · 2026-02-26T19:01:20Z

sunkit_spex/models/physical/nonthermal.py

+# def _check_input_type(spectral_axis):
+#     if isinstance(spectral_axis, SpectralAxis):
+#         energy_edges = spectral_axis.bin_edges
+#     else:
+#         warnings.warn(
+#             "As a SpectralAxis object was not passed, bin edges will be calculated as averages from the centers given.",
+#             UserWarning,
+#         )
+#         spectral_axis = SpectralAxis(spectral_axis, bin_specification="centers")
+#         energy_edges = spectral_axis.bin_edges
+
+#     return energy_edges


repeated code from before, so delete

settwi · 2026-02-26T19:01:42Z

sunkit_spex/models/physical/thermal.py

+        self.spectral_axis = spectral_axis

        if abundance_type != DEFAULT_ABUNDANCE_TYPE:
            mg, al, si, s, ar, ca, fe = _initialize_abundances(DEFAULT_ABUNDANCES[abundance_type])


does this include the abundance global state fix from main?

settwi · 2026-02-26T19:02:21Z

sunkit_spex/models/physical/thermal.py

+        if hasattr(temperature, "unit"):
+            return flux
+        return flux.value
+


should we support unitless model outputs? (existential question)

settwi · 2026-02-26T19:04:14Z

sunkit_spex/models/physical/thermal.py

+def _spectral_axis_process(temperature, energy_edges, spectral_axis):
+    if spectral_axis is not None:
+        if isinstance(spectral_axis, SpectralAxis):
+            if (
+                hasattr(spectral_axis, "_meta")
+                and isinstance(spectral_axis._meta, dict)
+                and "photon_axis" in spectral_axis._meta
+            ):
+                spectral_axis_clean = spectral_axis._meta["photon_axis"]
+                warnings.warn("Using user defined photon_axis for evaluation")
+            elif hasattr(spectral_axis, "_bin_edges"):
+                spectral_axis_clean = spectral_axis._bin_edges
+                warnings.warn("Spectrum object has no photon axis stored in meta. Using bin edges.")
+            else:
+                spectral_axis_clean = SpectralAxis._edges_from_centers(spectral_axis.value, spectral_axis.unit)
+                warnings.warn("Calculating bin edges from centers as only centers passed to SpectralAxis.")
+    else:
+        spectral_axis_clean = None
+
+    if hasattr(temperature, "unit"):
+        temperature = temperature.to(u.K)
+
+        if isinstance(energy_edges, SpectralAxis):
+            if (
+                hasattr(energy_edges, "_meta")
+                and isinstance(energy_edges._meta, dict)
+                and "photon_axis" in spectral_axis._meta
+            ):
+                energy_edges = energy_edges._meta["photon_axis"]
+            elif hasattr(energy_edges, "_bin_edges"):
+                energy_edges = energy_edges._bin_edges
+            else:
+                energy_edges = SpectralAxis._edges_from_centers(energy_edges.value, energy_edges.unit)
+
+        if spectral_axis is not None:
+            if not np.array_equal(energy_edges, spectral_axis_clean):
+                raise ValueError("Evaluation axis must matched initialised spectral axis.")
+            energy_edges = spectral_axis_clean
+            warnings.warn(
+                "User has initialised with a spectral axis, therefore model will be evaluated based on this.",
+                UserWarning,
+            )
+        else:
+            energy_edges = SpectralAxis._edges_from_centers(centers=energy_edges.value, unit=energy_edges.unit)
+
+    else:
+        temperature = (temperature * u.MK).to_value(u.K)
+
+        if spectral_axis_clean is not None:
+            if not np.array_equal(energy_edges, spectral_axis.value):
+                raise ValueError("Evaluation axis must matched initialised spectral axis.")
+            energy_edges = spectral_axis_clean.value
+            warnings.warn(
+                "User has initialised with a spectral axis, therefore model will be evaluated based on this.",
+                UserWarning,
+            )
+        else:
+            energy_edges = SpectralAxis._edges_from_centers(centers=energy_edges, unit=u.keV).value
+
+    return energy_edges, temperature


again, isn't the API that the SpectralAxis must be used? probably just check if not isinstance(axis, SpectralAxis): <throw an error>

settwi · 2026-02-26T19:04:40Z

sunkit_spex/spectrum/spectrum.py

-    @property
-    def spectral_axis_index(self):
-        return self._spectral_axis_index
+    # def __array_finalize__(self, obj):


delete commented code

jajmitchell added 19 commits July 8, 2025 18:28

Adds SRM units and passes Spectralaxis to thermal

36223af

Adds SRM units and passes Spectralaxis to thermal

Checks input is SpectralAxis object

c8b9b95

Checks input is SpectralAxis object, converts and warns if not

Adds check input function

c96bae5

Adds check input function

ThickTarget and ThinTarget use centers

84b07e8

ThickTarget and ThinTarget use centers

Albedo now takes SpectralAxis and centers

d9f16b5

Albedo now takes SpectralAxis and centers

Pre-commit

062c0ad

Pre-commit

scaling.py and models.py now function with centers

403b2ac

scaling.py and models.py now function with centers

Fixes scaling and albedo tests

46e3b93

Fixes scaling and albedo tests

Adds __array_finalize__ method to SpectralAxis

47eef10

Adds __array_finalize__ method to SpectralAxis

Fixes tests

9516b9d

Fixes tests

Fixes part of fitting_simulated_data.py

0e9103f

Fixes part of fitting_simulated_data.py

Reverts SRM model to main version

b3be7e9

Reverts SRM model to main version

Fixes fitting tests

ff4c977

Fixes fitting tests

Fixes test_models.py

12c39a0

Fixes test_models.py

Fixes Albedo example

0e49d45

Fixes Albedo example

Fix Albedo example 2

56c71c3

Fix Albedo example 2

Fixes examples

dde9e35

Fixes examples

Adds changelog

a0f2db5

Adds changelog

Pre_commit fix

8d11859

Pre_commit fix

jajmitchell added 2 commits July 25, 2025 17:24

Adds custom __call__ to ThermalEmission

1b53ad6

Adds custom `__call__` to `ThermalEmission`

Fixes test_thermal.py

8498325

Fixes test_thermal.py

jajmitchell requested review from KriSun95, natsat0919, samaloney and settwi July 28, 2025 13:56

Thermal model uses photo_axis from meta

8d4c1b1

Adds meta to SpectralAxis, thermal model can now take Spectrum object, Spectral axis, or array

jajmitchell added 7 commits September 10, 2025 17:02

Can pass array without initialising

54407a3

Can pass array without initialising

Fitting works

56b71aa

Fitting works

Tidy up

1349730

Tidy up

More tidy

2e17bd5

More tidy

Remove unused functions

b4c9782

Remove unused functions

Merge branch 'main' into pass-energy-centers

3945f91

Fixes continuum and line models

e32ab3d

Fixes continuum and line models

samaloney closed this Feb 26, 2026

samaloney reopened this Feb 26, 2026

settwi requested changes Feb 26, 2026

View reviewed changes

Conversation

jajmitchell commented Jul 8, 2025

This PR enables the physical models to be used with a SpectralAxis

Uh oh!

samaloney commented Jul 21, 2025

Uh oh!

jajmitchell commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jajmitchell commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jajmitchell commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

settwi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jajmitchell commented Jul 21, 2025 •

edited

Loading

jajmitchell commented Jul 22, 2025 •

edited

Loading

jajmitchell commented Jul 28, 2025 •

edited

Loading