Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Spectrum and CountSpectrum objects #142

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

samaloney
Copy link
Collaborator

Heavily based off of the Spectrum object in specutils but also significantly stripped down. I think since the API is a subset it would easy enough to adopt the full API from specutils in the future if we decide to go that way. Since we know we will have a X-ray count spectra added a specific class for this as will have different properties.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 26.43678% with 64 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@0a47673). Click here to learn what that means.

Files Patch % Lines
sunkit_spex/spectrum/spectrum.py 26.43% 64 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #142   +/-   ##
=======================================
  Coverage        ?   55.02%           
=======================================
  Files           ?       21           
  Lines           ?     3253           
  Branches        ?        0           
=======================================
  Hits            ?     1790           
  Misses          ?     1463           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samaloney
Copy link
Collaborator Author

@samaloney
Copy link
Collaborator Author

I think the work we are doing in stixpy (TCDSolar/stixpy/pull/109) might be relevant here too @DanRyanIrish

@DanRyanIrish
Copy link
Member

I should point out a likely future direction of NDCube is to add an attribute contained an NDCoords class, which provides in easier API for accessing coordinate info. A possible implementation can be seen in ndcube#710.

In order to make the Tabular WCS infrastructure work properly with edges, we would also need to finish ndcube#713.

Once, these features are available, the only advantage I see for a Spectrum class in sunkit-spex would be a simpler instantiation API which doesn't require a WCS to be constructed manually beforehand. Everything else we needed would be on NDCube.

Lowering the instantiation barrier for users is important. Alternatively to a Spectrum class, this could be done by providing a function, build_spectral_cube(), which takes a data quantity, spectral axis quantity, and other optional NDCube inputs, and outputs an NDCube with the correct format. Dummy world axes with pixel units could be assigned if the data array was multi-dimensional.

If we agree this is the way forward, then working on the above two ndcube PRs becomes the bulk of the work needed for a de-facto Spectrum data object.

@samaloney
Copy link
Collaborator Author

I should point out a likely future direction of NDCube is to add an attribute contained an NDCoords class, which provides in easier API for accessing coordinate info. A possible implementation can be seen in ndcube#710.

In order to make the Tabular WCS infrastructure work properly with edges, we would also need to finish ndcube#713.

Once, these features are available, the only advantage I see for a Spectrum class in sunkit-spex would be a simpler instantiation API which doesn't require a WCS to be constructed manually beforehand. Everything else we needed would be on NDCube.

Lowering the instantiation barrier for users is important. Alternatively to a Spectrum class, this could be done by providing a function, build_spectral_cube(), which takes a data quantity, spectral axis quantity, and other optional NDCube inputs, and outputs an NDCube with the correct format. Dummy world axes with pixel units could be assigned if the data array was multi-dimensional.

If we agree this is the way forward, then working on the above two ndcube PRs becomes the bulk of the work needed for a de-facto Spectrum data object.

What is the best way to get these changes, extern the whole of NDCube and add the changes from these PRs 😆

@samaloney samaloney marked this pull request as ready for review September 19, 2024 13:33
@samaloney
Copy link
Collaborator Author

So I think we should maybe just merge this so we a very basic spectrum object

@DanRyanIrish DanRyanIrish merged commit 375bb9a into sunpy:main Sep 19, 2024
11 checks passed
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.

4 participants