-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add operations to return WCS coordinates from argmin/argmax #680
Add operations to return WCS coordinates from argmin/argmax #680
Conversation
Where do we stand on this? You need review? Rebase? |
Rebase. Will do by the end of the day |
1448c8e
to
aa8d15d
Compare
Codecov Report
@@ Coverage Diff @@
## master #680 +/- ##
==========================================
+ Coverage 80.19% 80.34% +0.14%
==========================================
Files 24 24
Lines 5505 5552 +47
==========================================
+ Hits 4415 4461 +46
- Misses 1090 1091 +1
Continue to review full report at Codecov.
|
@astrofrog @keflavich This is ready! |
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.
I like this but I wonder if we should return all world coordinates - there are cases where the RA/Dec may not line up with the pixel axes, for example they may be rotated by 45 deg. So then if you called argmax_world(axis=1) it's not clear if what would be returned is RA or Dec. Picking the world coordinate with the same index as the pixel axis only make sense in the cases where pixel and world axes are aligned?
@astrofrog Is this presently going to return incorrect results in the case of a rotated WCS? If so, we should special-case this for aligned axes only (we have that ability in generalized WCS, right?), and punt the general case for now. I'm not sure how much use there is for the general case anyway. |
@astrofrog @keflavich Here's the check I added for correlated image -> WCS axes: https://github.com/radio-astro-tools/spectral-cube/pull/680/files#diff-5efbc7ba09eb93125b163d09f8107e93a24aac38ed8791ef0b17b5ffb2fbea18R486 My understanding of I'm wondering if the changes in this PR should be restricted to only the spectral axis. I'm now not sure that any other case is well defined |
Current failures will be fixed in #688 |
…xis=None cases that aren't defined for these operations
…ns to make 2D maps (max/min, argmin/max)
…world cannot handle
b06c187
to
eb56512
Compare
@astrofrog Can you have a quick look through the new changes? I've added checks for correlated pixel -> WCS axes. This otherwise ready to go, IMO |
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, though I have suggestions on the docstring.
also, there's a lot of duplicated code between argmax/argmin. Could we combine those into one with method='max'
/method='min'
and just pick between them, then have argmin_world
just return argminmax_world(method='max', **kwargs)
?
Co-authored-by: Adam Ginsburg <[email protected]>
Co-authored-by: Adam Ginsburg <[email protected]>
@keflavich -- Thanks for the suggestion. I've added I think this is ready to merge if these last tests pass. |
Revised version in #665 that does not depend on #656
Adds
SpectralCube.argmin_world
andSpectralCube.argmax_world
to return the WCS coordinates of the argmin/max along a dimension. I've generalized a snippet from @low-sky for this.The main use is creating "peak velocity" maps from a cube (i.e., velocity at peak intensity):