Draft
Conversation
…e trace . . . .. , . . code style .
…c method and modified the method's logic accordingly.
…nd mask_treatment.
…nd mask_treatment.
…ns' instead of 'pytest.raises' to test for an expected warning.
…duce.core._ImageParser' mixin class.
Renamed the `Image` class to `SRImage` for better clarity and added comprehensive validation for the dispersion and cross-dispersion axes. Introduced properties to provide reordered views of data, mask, and uncertainty arrays.
Redesigned the SRImage class to centralise image initialisation and sanity checks. Standardised axis handling across the class.
Replaced `_ImageParser` with `SRImage` in `Background` class to standardize image handling. Added utility functions in `SRImage` for masking and subtraction, and updated related methods and tests to accommodate these changes.
…RImage` class to simplify dimension handling. Refactored trace calculations in `tracing.py` to utilize these new properties.
… replacing the _ImageParser. Updated related tests and modules to accommodate SRImage-specific methods and streamlined code a bit.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #228 +/- ##
===========================================
- Coverage 83.37% 26.72% -56.66%
===========================================
Files 13 14 +1
Lines 1137 1220 +83
===========================================
- Hits 948 326 -622
- Misses 189 894 +705 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
The commit history of this PR is dirty. Please rebase. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This draft PR proposes one way to refactor image data handling in Specreduce. The PR introduces a
specreduce.image.SRImageclass that works as a (relatively thin) wrapper aroundastropy.nddata.NDDataRef. I chose to wrapNDDataRefinstead ofspecutils.Spectrum1Dbecause I think this is a more appropriate class for 2D images, especially when we start working on 2D wavelength calibration. The methods that produce 1D spectra still returnSpectrum1Dobjects.The main functionalities of
SRImageare:SRImageinitialiser combines the image parsing logic that was previously inspecreduce.core._ImageParserandspecreduce.extract.HorneExtract._parse_image.numpy.moveaxisinstead ofnumpy.transposein the case we want to support images withndim > 2in the future. (The class hascrossdisp_axisanddisp_axisproperties, which are fixed to 0 and 1, respectively.)apply_maskmethod that applies the mask to the image data using the logic introduced by @cshanahan1 in PR Add default and set to zero masking option for specreduce operations. #216, and ato_masked_arraymethod that applies the mask and returns the data as a masked array.The PR modifies the
Background,Tracing, andExtractionclasses to work with theSRImageclass. The refactor passes all the tests without modifications to the tests themselves (well, except the ones forHorneExtract, which is something I still need to work on), so the changes should not modify or break the functionality of any existing code using public methods.While it passes (nearly) all the tests, this PR is not ready to be merged. I've created it to raise discussion and hear everyone's opinion about the approach to the refactor. So, please comment on whether this is a sensible way to go or if you'd prefer something else.
If this approach gains sufficient support, I'll polish the code and prepare the PR ready to be merged. In any case, this will need to wait until PR #216 is merged (the code is based on the masking work by @cshanahan1 in #216), and I still need to document the code properly and rewrite some of the
HorneExtracttests to work with the new image handling logic, but my aim is that theHorneExtractionclass itself will work identically as before without any visible changes to the end-user.