Skip to content

Add sunkit spex data loaders - #227

Draft
KriSun95 wants to merge 20 commits into
sunpy:mainfrom
KriSun95:add-sunkit-spex-data-loaders
Draft

KriSun95 wants to merge 20 commits into
sunpy:mainfrom
KriSun95:add-sunkit-spex-data-loaders

Conversation

@KriSun95

@KriSun95 KriSun95 commented Sep 17, 2026 •

Copy link
Copy Markdown

I'm opening as a work-in-progress draft but more than happy for comments.

There is a bunch of instrument specific code in the sunkit-spex X-ray spectral fitting package and on local machines to load in spectra from those instruments and fit it. This code does not fall into the scope for sunkit-spex and does not have a home anywhere else.

Therefore, this is the first PR at moving, and updating, the instrument specific code to sunkit-instruments. The main focus here is for the NuSTAR observatory, but other PRs for other instrument code that doesn't have a home will likely come soon.

To do:

  • Remove references to RHESSI in this PR (should be its own PR)
  • Finish writing tests for the main NuSTAR spectrum object code
  • Create an example in the example gallery to show what the NuSTAR object should be used for

@KriSun95

Copy link
Copy Markdown
Author

Pre-commit fails because of my lambda functions in test_nustar.py (fine) but it also keeps changing "livetime" to "lifetime" which is problematic :(

Comment thread pyproject.toml
Comment on lines +27 to +28
"sunkit_spex>=0.5.0",
"ndcube>=2.4.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.

I'm not sure how this would work if sunpy_spex also depends on sunkit_inst maybe it's a non-issue

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm not sure either but I was thinking, will sunkit_spex depend on sunkit_inst?


__all__ = ["regroup_any_array", "rebin_rmf"]

def regroup_any_array(data:np.ndarray|u.Quantity, old_bins:np.ndarray|u.Quantity, new_bins:np.ndarray|u.Quantity, combine_by:str|None=None):

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.

So I tink this should live in sunkit-spex on an ARM / RMF object and use/extend NDCube API to do the rebin

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm not sure where my thoughts are on that.

My main thought is as long as it's easily gettable and I could see why a user would want to rebin the data prior to fitting, or maybe after for plotting, but I'm not sure of a use case that the rebinning should be done during fitting.

I feel a user should get the data in whatever stage they want and then fit that. In that case, I feel that the object handling the spectral data should also be able to handle the rebinning. I would worry that an instrument would need rebinning for either the ARF/RMF to be handled in a weird way meaning that an ARF/RMF object's rebin method would need to be edited on a instrument basis anyway.

Ultimately, it can go wherever but I just wanted to put some thoughts out there.

@KriSun95

KriSun95 commented Oct 2, 2026

Copy link
Copy Markdown
Author

@samaloney , I think this is starting to take its final shape. The only outstanding thing for me to change is _exactly_the form of the spectrum object. Other than that, I think it a case of reviewing, tearing this apart, and getting it merged.

I'd imagine that the NuSTAR files I'm hosting to download for the example would be better hosted elsewhere and I'm really happy to take guidance on that.

Since this is actually quite nice to use to look at NuSTAR data outside of spectral fitting, I'll also get other NuSTAR folk to have a look at some point.

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