Repository navigation
Fixes to sgr coordinates example - #5541
Conversation
pllim
added
Affects-dev
|
@adrn - can you rebase to get the proper CircleCI configuration? |
1c8316f to
b0a6a44
Compare
|
Nice! Glad I looked over the recent PR's before I started trying to fix up #5210 😉 will review shortly. |
|
@astrofrog Rebased - build failure is just a canceled OSX build |
| # This is done at the module level, since it will be used by both the | ||
| # transformation from Sgr to Galactic as well as the inverse from Galactic to | ||
| # Sgr. Now we can define our first transformation function: | ||
| # This rotation matrix is defines at the module level, since it will be used |
| # Sgr. Now we can define our first transformation function: | ||
| # This rotation matrix is defines at the module level, since it will be used | ||
| # by both the transformation from Sgr to Galactic as well as the inverse from | ||
| # Galactic to Sgr. Now we can define our first transformation function: |
There was a problem hiding this comment.
How about:
# Since we already constructed our matrix above, and the inverse of a
# rotation matrix is just its, transpose, the required transformation functions
# are very simple.
| # code <http://www.astro.virginia.edu/~srm4n/Sgr/code.html>`_. Note that in this | ||
| # case, both coordinate systems are heliocentric, so we can simply copy any | ||
| # distance from the `~astropy.coordinates.Galactic` object. | ||
| # we simply construct a rotation matrix using the Euler angles (and the |
There was a problem hiding this comment.
This is no longer true, maybe move the comment to where the actual matrix is constructed?
|
|
||
| fig,axes = plt.subplots(2, 1, figsize=(6,12), | ||
| subplot_kw={'projection': 'aitoff'}) | ||
| fig, axes = plt.subplots(2, 1, figsize=(8, 10), |
There was a problem hiding this comment.
Now that we have WCSAxes inside astropy, should it be used here? It seems a perfect place for a nice example...
(But don't do it if it takes too much time.)
There was a problem hiding this comment.
I think not, because the whole point of the example is to demonstrate the use of a custom coordinate system, which WCSAxes I believe does not support (because it's not in the FITS-WCS standard). And to try to get it to would require a lot more code that I think would distract from the point of this example?
But it's probably a good idea to have another example of how you might do this in WCSAxes...?
|
@mhvk Good ideas -- I'll get these changes in by tonight. |
|
👍 |
|
This looks good, so I'll merge. I do note that one could use units a bit more throughout, but that might as well be done later (see #5555). |
5 participants
This is a replacement for #5210 that changes the transformation to the Sagittarius plane coordinate from a function transform to a matrix transform. This simplifies the transform code and makes it more consistent with the other implemented transformations.
cc @eteq @mhvk