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

Image data handling refactor #228

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from
Draft

Conversation

hpparvi
Copy link
Contributor

@hpparvi hpparvi commented Nov 11, 2024

This draft PR proposes one way to refactor image data handling in Specreduce. The PR introduces a specreduce.image.SRImage class that works as a (relatively thin) wrapper around astropy.nddata.NDDataRef. I chose to wrap NDDataRef instead of specutils.Spectrum1D because 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 return Spectrum1D objects.

The main functionalities of SRImage are:

  • The SRImage initialiser combines the image parsing logic that was previously in specreduce.core._ImageParser and specreduce.extract.HorneExtract._parse_image.
  • The initialiser standardises the image axes so that the cross-dispersion axis is always the first axis and the dispersion axis is always the second axis. The primary motivation for this is to ensure the rest of the code doesn't need to worry about the ordering of the axes. Also, the re-ordering is done using numpy.moveaxis instead of numpy.transpose in the case we want to support images with ndim > 2 in the future. (The class has crossdisp_axis and disp_axis properties, which are fixed to 0 and 1, respectively.)
  • The class contains an apply_mask method 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 a to_masked_arraymethod that applies the mask and returns the data as a masked array.

The PR modifies the Background, Tracing, and Extraction classes to work with the SRImage class. The refactor passes all the tests without modifications to the tests themselves (well, except the ones for HorneExtract, 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 HorneExtract tests to work with the new image handling logic, but my aim is that the HorneExtraction class itself will work identically as before without any visible changes to the end-user.

cshanahan1 and others added 25 commits September 18, 2024 15:51
…c method and modified the method's logic accordingly.
…ns' instead of 'pytest.raises' to test for an expected warning.
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.
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 35.78595% with 192 lines in your changes missing coverage. Please review.

Project coverage is 26.72%. Comparing base (6b8e995) to head (eace0ae).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
specreduce/image.py 29.86% 101 Missing ⚠️
specreduce/tracing.py 41.79% 39 Missing ⚠️
specreduce/extract.py 40.42% 28 Missing ⚠️
specreduce/background.py 40.00% 21 Missing ⚠️
specreduce/utils/utils.py 25.00% 3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (6b8e995) and HEAD (eace0ae). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (6b8e995) HEAD (eace0ae)
2 1
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.
📢 Have feedback on the report? Share it here.

@pllim
Copy link
Member

pllim commented Nov 11, 2024

The commit history of this PR is dirty. Please rebase.

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.

3 participants