pyuvdata icon indicating copy to clipboard operation
pyuvdata copied to clipboard

Telescope frame

Open aelanman opened this issue 2 years ago • 3 comments

Description

Introduces a new parameter telescope_frame to UVData, which can be either "ITRS" (default) or "MCMF" (Moon-Centered Moon-Fixed). This new parameter indicates the coordinate frame the telescope_position is defined in. Several utility functions and UVData methods are configured to use this parameter:

  • ENU_from_ECEF
  • ECEF_from_ENU
  • LatLonAlt_from_XYZ
  • XYZ_from_LatLonAlt
  • get_lst_for_time

The package lunarsky is needed to use the MCMF frame. It's been added as an optional dependency.

Motivation and Context

pyuvsim relies on pyuvdata for computing uvw coordinates and correctly transforming antenna positions. While pyradiosky and pyuvsim both support lunarsky coordinate frame definitions, so far pyuvdata has not included this. Once this is merged, a corresponding small change to pyuvsim should fix https://github.com/RadioAstronomySoftwareGroup/pyuvsim/issues/408.

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)
  • [ ] Documentation change (documentation changes only)
  • [ ] Version change
  • [ ] Build or continuous integration change

Checklist:

  • [x] I have read the contribution guide.
  • [x] My code follows the code style of this project.

New feature checklist:

  • [x] I have added or updated the docstrings associated with my feature using the numpy docstring format.
  • [ ] I have updated the tutorial to highlight my new feature (if appropriate).
  • [x] I have added tests to cover my new feature.
  • [ ] All new and existing tests pass.
  • [ ] I have updated the CHANGELOG.

aelanman avatar Aug 10 '22 18:08 aelanman

Codecov Report

Merging #1199 (60f6f7d) into main (183597b) will increase coverage by 0.00%. The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1199   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files          32       32           
  Lines       17405    17472   +67     
=======================================
+ Hits        17389    17456   +67     
  Misses         16       16           
Impacted Files Coverage Δ
pyuvdata/utils.py 100.00% <100.00%> (ø)
pyuvdata/uvdata/uvdata.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 183597b...60f6f7d. Read the comment docs.

codecov[bot] avatar Aug 10 '22 20:08 codecov[bot]

I would think that the LST should be set to the RA at zenith.

bhazelton avatar Aug 11 '22 15:08 bhazelton

@bhazelton My changes to the tests should cover the new cases, but I'm not sure how to change the circleci config to install lunarsky and run them.

aelanman avatar Aug 11 '22 22:08 aelanman

@aelanman sorry for the delay. I just added lunarsky to some of the test environments. It looks like there's now only one uncovered line. I need to make some more updates for it but I needed to know if there was a minimum version of lunarsky to use? Also, it looks like you need to rebase the branch.

bhazelton avatar Sep 17 '22 00:09 bhazelton

@bhazelton Thanks for fixing that! I've made some changes to cover the remaining missing lines, and rebased off main. For lunarsky, I would recommend a minimum version of 0.1.2, which is the third release.

aelanman avatar Sep 17 '22 22:09 aelanman

@aelanman we discussed this in last week's telecon and we're broadly supportive but we don't really like the approach of replacing the astropy SkyCoord with the lunarsky one if lunarsky is installed. We'd prefer to import the lunarsky one as a different name and only use it if needed (i.e. if the telescope is on the moon). Similarly we'd like to avoid using the mock classes in case lunarsky is not installed. I'm happy to tackle these changes if you're too busy on other projects.

bhazelton avatar Oct 04 '22 00:10 bhazelton

@bhazelton I've made some changes which ensure SkyCoord and Time are imported with separate names for astropy and lunarsky, and the lunarsky versions (LSkyCoord and LTime) are only used when a lunarsky-related class is used. It also removes the dummy classes. Fortunately, the changes here used lunarsky sparingly and mostly defined new XYZ/LatLonAlt transformations by using basic erfa functions. There was a slight hit in coverage; I'll have to fix that later.

The main reason for replacing the class globally is that it can be hard to predict where each form will be needed. The lunarsky classes inherit from their astropy equivalents, but just add support for the new MoonLocation class, etc. In principle, they should be identical (including passing tests of "isinstance"). However, it can be confusing if you look at the, say, "SkyCoord" instance and its class is lunarsky.sky_coord.SkyCoord.

aelanman avatar Oct 04 '22 17:10 aelanman