Repository navigation
WCS should accept / return coordinates objects #812 - #2210
QuanTakeuchi wants to merge 2 commits into
Conversation
|
Is this approach fine? |
There was a problem hiding this comment.
Why are you converting the angles to strings and back again?
There was a problem hiding this comment.
We need to get a float value from the angle object. And parse_angle takes string as an input. Besides dms_to_degrees needed each value seperately. That's the reason I did it that way
Is there a better way to do this?
There was a problem hiding this comment.
I think you can just do xy.lonangle.degrees.
There was a problem hiding this comment.
Be careful also -- add update your test accordingly -- that SphericalCoordinates objects can contain/be an array of coordinates, not just a single coordinate.
There was a problem hiding this comment.
I did try .degrees in the beginning, but it didn't work so I assumed that the object has no such attribute. Anyways after your suggestion I looked into it and found that there is no 's' in degree. Thanks for pointing out though.
And sure I will see to it that the it takes a list of coordinates as input.
|
There's a couple of problems here:
|
|
Ok I will modify it to take this into consideration. |
|
@mdboom Tried to include all the suggestions you gave. What all problems can arise from this implementation? |
|
@QuanTakeuchi - just a side comment. It looks like you are merging the master branch into the feature branch you are using here, but this causes many 'merge' commits to appear in the list of commits. You can avoid this by rebasing the feature branch on master instead of merging master into the feature branch. You can also get rid of those commits now by rebasing on the latest upstream master, then force pushing to the remote branch. |
|
@astrofrog I tried to rebase. I was expecting that now it would show just the commits I made on this feature branch, but it includes the commits on the upstream master branch as well. Is this how it should work, or I have done the rebasing incorrectly? |
|
@QuanTakeuchi - you should rebase with: Does this work? Once you do this the latest upstream commits will appear in your history, but should not appear in the pull request. |
|
@astrofrog Thanks. It worked. But what about the changes that I made in this pull request? Are they fine? |
|
@QuanTakeuchi - I think we will need to discuss this a little with other developers before merging this - the issue is that for example if the WCS has a specific coordinate system (for example ICRS) and Galactic coordinates are passed, we need to intelligently understand how to convert these. |
|
@astrofrog Okay, got it. We can compare both the input coordinate system and WCS coordinate system and convert the input value to the latter. Is there any problem in doing it this way? |
|
@QuanTakeuchi - I don't know if there's an easy way to figure out the relation between the WCS coordinate system and the coordinate system from astropy.wcs at the moment. Maybe @mdboom can comment on this? |
There was a problem hiding this comment.
The world coordinates can be more than two-dimensional -- for example, (lat, lon, spec), or it could even be (lat, spec). So the correct solution might be to create a zeroed array for all of the dimensions, fill in xy.lonangle.degree in index self.wcs.lng and xy.latangle.degree in index self.wcs.lat. Alternatively, you could check that the world coordinate dimensions are 2 (self.wcs.naxis == 2) and that the wcs.lat and wsc.lng are 0 and 1 (in either order), and if not, raise an exception.
There was a problem hiding this comment.
Ok. Sorry I should have read about world coordinates first, and thought of this possibilty.
Anyways I will correct this.
|
As for matching the input coordinate system to the WCS coordinate system: There are a few attributes on I don't think I know enough about coordinate systems to say if that's enough information for what we need, but hopefully that's a good starting point. |
|
@mdboom I will go through this and see what i can do about it. |
|
@QuanTakeuchi - just to check, is this still something you are interested in working on? If so, I can provide some updated instructions (there are helper functions now in |
|
Oh sorry.. I had forgotten about it for to my academics. |
|
I am still interested though. It's My vacation time now. Sure i would like to resume writing on it. What are those functions? |
|
@QuanTakeuchi - I need to add one of those functions to |
|
@QuanTakeuchi - I've opened the following pull request: #2885 - with that in hand, it should be easy to then implement this pull request :) |
|
@QuanTakeuchi: I'm going through and triaging pull requests. I'd still be happy to have this functionality in astropy if we can work out the kinks. For now, though, given the imminent 1.0 release, I am going to remove the 1.0 milestone. Do you still have interest in completing this work? |
|
I am sorry. I couldn't get this through. I won't be able to pursue it now. Sorry for the inactivity |
In response to Issue #812