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.

Sign up for GitHub

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

Jump to bottom

Return quantity objects in pix2world and world2pix transformations #276

Merged
merged 7 commits into from
Sep 11, 2018
Merged

Return quantity objects in pix2world and world2pix transformations #276

merged 7 commits into from
Sep 11, 2018

Conversation

nmearl
Copy link
Contributor

@nmearl nmearl commented Aug 17, 2018

From a conversation with @eteq, pixel_to_world should return a Quantity object instead of the Quantity being created when the spectral_axis is evaluated in the Spectrum1D object.

@nmearl nmearl added this to the v0.6.0 milestone Sep 5, 2018
@eteq
Copy link
Member

eteq commented Sep 7, 2018

(note the test failure looks real, @nmearl , so I'm waiting on that to review this)

@nmearl nmearl force-pushed the wcs-transform-unit branch 2 times, most recently from 00ed223 to 3b2b1e2 Compare September 7, 2018 22:31
@nmearl
Copy link
Contributor Author

nmearl commented Sep 7, 2018 •
edited

@eteq Do you know if world_to_pixel should return a Quantity with no unit (according to APE14)?

@nmearl nmearl requested a review from eteq September 8, 2018 00:59
@eteq
Copy link
Member

eteq commented Sep 11, 2018 •
edited

@nmearl - as I mentioned yesterday out-of-band, the canonical place to look is the implementation for FITS-WCS. That's astropy/astropy#7325 + astropy/astropy#7326 . There it turns out the pixel coordinates are not Quantity objects, rather raw arrays (which is not what I thought in our conversation before).

eteq
eteq requested changes Sep 11, 2018
View reviewed changes
Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this still needs tests that check explicitly if the output is a Quantity. Also it doesn't look like world_to_pixel is tested? (although maybe it was awaiting an answer to the question of what that should return, which I pointed out above)

.eggs/README.txt Outdated
@@ -0,0 +1,6 @@
This directory contains eggs that were downloaded by setuptools to build, test, and run plug-ins.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should not be included. In 9448fe2 I added .eggs to gitignore, but in the meantime this should not be checked in in this PR.

@nmearl
Copy link
Contributor Author

nmearl commented Sep 11, 2018

@eteq Tests have been added (it tests pixel_to_world and world_to_pixel for both fits wcs and gwcs); file has been removed.

@eteq
Copy link
Member

eteq commented Sep 11, 2018

LGTM now - thanks @nmearl !

@eteq eteq merged commit 1bb026e into astropy:master Sep 11, 2018
SaOgaz pushed a commit that referenced this pull request Mar 25, 2019
Merge pull request #276 from drdavella/pin-mpl-2.1
16b33bd 
Pin MPL to 2.1 for now to work around glue bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eteq eteq eteq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nmearl @eteq @javerbukh