Repository navigation
Added new East-North-Up and UVW frames (from radio astronomy) - #6882
Joshuaalbert wants to merge 6 commits into
Conversation
…ames and unit tests
|
Hi there @Joshuaalbert 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. I noticed the following issue with this pull request:
Would it be possible to fix this? Thanks! If there are any issues with this message, please report them here. |
astrofrog
changed the title
|
@Joshuaalbert - it looks like there are some real test failures due to missing imports. Could you fix these? Note that you can run the tests locally using: (the pip install command is only needed the first time you try and run the tests) |
There was a problem hiding this comment.
Sorry that this has taken so long, @Joshuaalbert - I lost track of the earlier one.
Generally looks good, but I left some comments where I think the docstrings need more info. Note also that I'm a bit confused why the ENU coordinates are here. My read of #5300 is that that's just a rotation/mirroring of AltAz, which isn't necessary. On the other hand, UVW definitely is. And isn't it the case that UVW with phase=0 is the same? Or am I mis-understanding here?
Also, there needs to be an entry in the changelog for this...
|
|
||
| class ENU(BaseCoordinateFrame): | ||
| """ | ||
| A coordinate or frame in the East-North-Up (ENU) system. |
There was a problem hiding this comment.
I think a bit more specifics about this system are called for. It's centered on the surface of the earth, right? (at location) and the east/north/up are based on the obstime orientation? So is it different from AltAz in any way other than the names? If so, it should be tied to AltAz rather than ITRS, right?
There was a problem hiding this comment.
(or if I'm misunderstanding and it's different from AltAz, that difference should be stated clearly here in the docstring)
There was a problem hiding this comment.
So the ENU system and AltAz have similar coordinates except AltAz is left-handed. Azimuth increases from North to East. AltAz is the same as North-East-Down. Otherwise the origin is fixed to location like AltAz. I included ENU because I had found it useful before, however I'm not married to the idea of including it in astropy. However, I think the UVW is certainly useful for many people. I chose to tie the graph transform to ITRS for simplicity in terms of rotations but now that I think about it, simply flipping Up to Down and rearranging ENU to NED is simpler.
There was a problem hiding this comment.
My main concern here is that there's some subtle differences between AltAz and the ENU implemented here - the most obvious is the possibility of atmospheric refraction, but there are more subtle things (like the different code pathways having different machine precision since AltAz uses erfa). So Might it make more sense for this to be transformed from AltAz? That comes with some baggage, though, of requiring an atmospheric refraction.
So I'm tempted to instead say either drop ENU entirely or have it be tied to UVW. (I think the latter is what you were suggesting in your last sentence here?)
There was a problem hiding this comment.
So I've decided I should just drop ENU as it's not strictly an astro frame in common use. In anycase, it's the same as UVW with phase = zenith.
|
|
||
| class UVW(BaseCoordinateFrame): | ||
| """ | ||
| A coordinate or frame in the UVW system, the conventional radio astronomy frame. |
There was a problem hiding this comment.
Similarly, I think this needs just a sentence or paragraph or something on what the u,v, and w coordinates are. While it's conventional in radio astronomy, the docs still should be comprehensible to non-radio astronomers.
This could also go in the Parameters section if you think that might be clearer.
There was a problem hiding this comment.
The convention for UVW is well stated here https://casa.nrao.edu/Memos/CoordConvention.pdf, which is also where I got the unit tests from.
When phase=0 (by which I understand to mean ra=0 and dec=0) the z-axis of UVW points at the vernal equinox (minus the LST). ENU is equivalent to UVW(phase = (ra=local_sidereal_time, dec=latitude)), which is similar to AltAz (see above).
More intuitively, the UVW is the projected ENU frame as seen from the source (the pointing given by phase). The convention is the one used by radio astronomers to specify the baselines of interferometer elements.
I'll add to the docstring.
| @frame_transform_graph.transform(FunctionTransform, UVW, ITRS) | ||
| def uvw_to_itrs(uvw_coo, itrs_frame): | ||
| lon, lat, height = uvw_coo.location.to_geodetic('WGS84') | ||
| lst = AltAz(alt=90*u.deg,az=0*u.deg,location=uvw_coo.location,obstime=uvw_coo.obstime).transform_to(ICRS).ra |
There was a problem hiding this comment.
Can't this just be computed more easily using Time.sidereal_time? http://docs.astropy.org/en/stable/api/astropy.time.Time.html#astropy.time.Time.sidereal_time
There was a problem hiding this comment.
Didn't know of that method. Will definitely use it.
There was a problem hiding this comment.
There is a notable problem which I can't understand.
Pytest is failing at lst = time.sidereal_time('mean',10*u.deg) with the below error, but when I run these two lines in the shell it works fine...
Pytest doesn't seem to like things.
time = at.Time("2017-01-26T17:07:00.000",format='isot',scale='utc')
lst = time.sidereal_time('mean',10*u.deg)
produces IERSRangeError: (some) times are outside of range covered by IERS table.
It happens because in function _check_interpolate_indices(self, indices_orig, indices_clipped, max_input_mjd)
| z = p[2], | ||
| copy=False) | ||
| return itrs_frame.realize_frame(rep) | ||
| @frame_transform_graph.transform(FunctionTransform, UVW, UVW) |
There was a problem hiding this comment.
(PEP8 minor nitpick): needs a blank line above
removed East North Up frame, changed tests to match.
|
There was a bug in the bot, it should not have been closed without the warning so re-opening. |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 7 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 consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. You may also consider sending a reminder e-mail about it to the astropy-dev mailing list. If you believe I commented on this pull request incorrectly, please report this here. |
|
@Joshuaalbert - would you like to finish this PR? If not, I can take over, as I would like to have the UVW frames. Currently, there are still tests of |
|
Is there another common name for the UVW frame? We use UVW as Galactic velocity components, so there could be some confusion. |
|
UVW are the classical letters given to the baseline coordinates. I don't think there are other conventions in use. |
|
@mhvk You may continue this PR. It would take a rewrite if I were to readdress it. Specifically, I would need to pay very strict attention to the definition of UVW used by the VLBI people. It's not enough to simply use East-North-Up for precision radio astronomy. The coordinates really depend on the relative motion of the observer. |
|
⏰ 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. |
|
@mhvk - are you interesting in taking this over as you suggested? If so, it might be best to open a new PR rather than re-open this one? |
|
Yes, I am interested, but given @Joshuaalbert's comment above it would seem a rewrite is needed, so leaving this closed is fine. I created #7766 to remind us. |
|
@mhvk This would be so valuable for more open access to radio interferometry algorithm development. |
|
@Joshuaalbert Would it possible to limit the scope of this to connected interferometers? Maybe the name of the system could be more specific or it can raise a warning if used for VLBI. |
|
@caseyjlaw - point taken about this being useful - I really do hope to still get around to it (if you have time, do feel free to do it, though...). I do think, though, that we should stick to whatever the precise and proper standard is, and not try to have a shortcut that sort-of works for some not so well-defined conditions. And I don't think it will be that much harder to do it right - all the pieces are definitely in place. Anyway, probably best to move further discussion to #7766 - closed issues are not the best place to discuss plans! |
…to InterferometricVisibility
Description edited by @astrofrog: this is a rebased version of #5706 - the original description from that PR is as follows:
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.