Skip to content

Conversation

@samaloney
Copy link
Member

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
Member Author

@samaloney
Copy link
Member 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
Member 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
Member 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
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