Repository navigation
Added an east north up frame object - #5706
Joshuaalbert wants to merge 6 commits into
Conversation
This contains a new representation (ENU) for the East-North-Up system at a given observer location and obstime. It gives the transformations between ENU and the ITRS frames for the graph transformation. The use case I wrote it for was for ray tracing.
There was a problem hiding this comment.
Thanks for your contribution. I left some general comments. Subpackage manager should give this a more thorough review.
In addition to specific comments below: PEP8 errors such as inconsistent indentations and too few/many blank lines. Remove commented codes (unused variables, print functions).
| from astropy.coordinates.baseframe import (BaseCoordinateFrame, FrameAttribute, | ||
| TimeFrameAttribute, QuantityFrameAttribute, | ||
| RepresentationMapping, EarthLocationAttribute, | ||
| frame_transform_graph) |
| @@ -0,0 +1,202 @@ | |||
| # An East-North-Up right handed coordinate system | |||
|
|
||
| class ENU(BaseCoordinateFrame): | ||
| """ | ||
| Written by Joshua G. Albert - [email protected] |
There was a problem hiding this comment.
I don't think I have seen direct authorship claim in a docstring in Astropy code-base. This can complicate things since Astropy is open-source and we don't want to maintain exactly who worked on which piece of code in the docstrings. Authorship is evident via git blame and other GitHub statistics tools.
|
|
||
| Notes | ||
| ----- | ||
| This is useful as an intermediate frame between ITRS and UVW for radio astronomy |
There was a problem hiding this comment.
Maybe add an example in the user-facing doc of such usefulness?
|
|
||
| obstime = TimeFrameAttribute(default=None) | ||
| location = EarthLocationAttribute(default=None) | ||
| #pressure = QuantityFrameAttribute(default=0, unit=u.hPa) |
There was a problem hiding this comment.
If they are not used, better to just delete them. Otherwise, it will just confuse people.
|
|
||
| @frame_transform_graph.transform(FunctionTransform, ENU, ITRS) | ||
| def enu_to_itrs(enu_coo, itrs_frame): | ||
| #p = itrs_frame.cartesian.xyz.to(u.m).value |
There was a problem hiding this comment.
Again, delete unused variables? Also, no docstring.
| # covered | ||
| return from_coo.transform_to(ITRS(obstime=from_coo.obstime,location=from_coo.location)).transform_to(to_frame) | ||
|
|
||
| if __name__ == '__main__': |
There was a problem hiding this comment.
Move test to a proper test module that can be discovered and run by pytest.
Addresses comments
|
Addressed the comments. |
|
@pllim Okay all checks passed now and your comments implemented. |
@Joshuaalbert - This will definitely need unit tests and documentation, but probably it's OK to wait for a review from the module maintainers first. |
|
Note that this closes #5300 (I'm planning to review this, hopefully soon) |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 8 months. I plan to close this in a month if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please close this and open an issue instead to revisit in the future. Maintainers may also choose to add If you believe I commented on this issue incorrectly, please report this here. |
|
⏰ Time's up! ⏰ I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
|
@eteq - if you are able to review this, do you want to re-open? |
|
This was re-opened as #6882 |
This contains a new representation (ENU) for the East-North-Up (right handed) system at a given observer location and obstime.
It gives the transformations between ENU and the ITRS frames for the graph transformation.
The use case I originally wrote this for was for ray tracing.