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.

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

utils.py has error in enu_to_ecef / enu_to_ecef on the moon #1351

Closed
jpober opened this issue Oct 24, 2023 · 1 comment · Fixed by #1376
Closed

utils.py has error in enu_to_ecef / enu_to_ecef on the moon #1351

jpober opened this issue Oct 24, 2023 · 1 comment · Fixed by #1376
Labels

Comments

@jpober
Copy link
Member

jpober commented Oct 24, 2023

There is an indexing error on lines 1508 & 1576 . We think it should be [1,2,0] but aren't 100% sure.

That said, the bigger issue is that this code is totally separate from the earth-based code. We should let ECEF_from_ENU (in the cython) take a frame. Right now, it has an if/else for earth vs moon, and the moon block has completely independent code. To make this work, XYZ_from_lat_lon_alt will need to accept the frame and then use lunar values for things like gps_n etc. It may be that we can get those values from lunarsky and, if we assume spherical symmetry, a lot of them become redundant.

@jpober jpober added the bug label Oct 24, 2023
@kartographer
Copy link
Contributor

Just to provide a little example here, some "test code" from the current version of main (run in an python shell):

In [1]: import pyuvdata.utils as uvutils

In [2]: uvutils.ECEF_from_ENU([0.001, 0, 0], 0, 0, 0, frame="ITRS")
Out[2]: array([6.378137e+06, 1.000000e-03, 0.000000e+00])

In [3]: uvutils.ECEF_from_ENU([0.001, 0, 0], 0, 0, 0, frame="MCMF")
Out[3]: array([1.7371e+06, 0.0000e+00, 1.0000e-03])

A small East offset is behaving like I expected for ITRS (shifting along the Y-axis), but not for MCMF (shifts along the Z-axis).

@mkolopanis mkolopanis mentioned this issue Jan 18, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants