-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Fix pixel region index retrieval #1001
Conversation
As discussed, this could use more tests to cover edge cases. What if region is completely out of bounds? Only 1 pix overlap? Within bounds but somehow falls between the gaps of sampled points? |
If you rebase after #1002 is merged, the CI should be green again unless this PR broke something. |
a6cd989
to
cfb8526
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1001 +/- ##
==========================================
+ Coverage 69.93% 69.98% +0.04%
==========================================
Files 64 64
Lines 4321 4331 +10
==========================================
+ Hits 3022 3031 +9
- Misses 1299 1300 +1 ☔ View full report in Codecov by Sentry. |
ed9f100
to
634738f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, left a couple comments. Mainly I think the check on ascending pixel axis should go in SpectralAxis.__new__
rather than appearing twice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thanks!
Thanks Ricky and Pey-Lian! |
When a
u.pix
defined region attempted toextract_region
on a Spectrum1D defined with a spectral axis defined withu.pix
, the region index bound detection algorithm failed to resolve the correct indices, instead returning the actual value of the pixel quantity of the spectral region. This resulted in an empty list being returned due to the slicing indices being incorrect.Quick test snippet:
should return:
whereas on
main
:tox
test failures appear to be unrelated, possibly related to the same issues we've seen in spacetelescope/jdaviz#1901. Related tests are all testing locally