Open samaloney opened 5 months ago
Attention: Patch coverage is 26.43678%
with 64 lines
in your changes are missing coverage. Please review.
:exclamation: 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 :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I think the work we are doing in stixpy (TCDSolar/stixpy/pull/109) might be relevant here too @DanRyanIrish
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.
I should point out a likely future direction of
NDCube
is to add an attribute contained anNDCoords
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 insunkit-spex
would be a simpler instantiation API which doesn't require a WCS to be constructed manually beforehand. Everything else we needed would be onNDCube
.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 optionalNDCube
inputs, and outputs anNDCube
with the correct format. Dummy world axes with pixel units could be assigned if thedata
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-factoSpectrum
data object.
What is the best way to get these changes, extern the whole of NDCube and add the changes from these PRs 😆
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.