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

Handle lunar ellipsoids #1417

Merged
merged 15 commits into from
Mar 25, 2024
Merged

Handle lunar ellipsoids #1417

merged 15 commits into from
Mar 25, 2024

Conversation

bhazelton
Copy link
Member

@bhazelton bhazelton commented Mar 23, 2024

Description

Support for various lunar ellipsoids was recently added in lunar sky 0.2.2. This handles those changes and is required for pyuvsim moon simulations to work.

To keep track of this information, I added a new lunar_ellipsoid attribute to LocationParameter objects (similar to how we handled the frame attribute) and I added handling to pipe it into and out of all the necessary utility functions.

Note that lunar sky now requires astropy >= 6.0 so I updated that requirement, which lead to a bunch of other knock-on changes to our minimum dependencies. Most notably, astropy 6.0 doesn't support python 3.9, so I dropped it from our test suite.

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • 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:

New feature checklist:

  • 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).
  • I have added tests to cover my new feature.
  • All new and existing tests pass.
  • I have updated the CHANGELOG.

Copy link

codecov bot commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.92%. Comparing base (e59eb84) to head (c3e6f60).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1417   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files          37       37           
  Lines       20807    20871   +64     
=======================================
+ Hits        20791    20855   +64     
  Misses         16       16           
Files Coverage Δ
pyuvdata/parameter.py 100.00% <100.00%> (ø)
pyuvdata/utils.py 100.00% <100.00%> (ø)
pyuvdata/utils.pyx 100.00% <100.00%> (ø)
pyuvdata/uvdata/initializers.py 100.00% <100.00%> (ø)
pyuvdata/uvdata/ms.py 99.89% <ø> (ø)
pyuvdata/uvdata/mwa_corr_fits.py 100.00% <100.00%> (ø)
pyuvdata/uvdata/uvdata.py 100.00% <ø> (ø)
pyuvdata/uvdata/uvfits.py 100.00% <100.00%> (ø)
pyuvdata/uvdata/uvh5.py 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

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

Copy link
Contributor

@kartographer kartographer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this is looking good -- mostly minor comments and one potentially more significant one (assuming you agree w/ my assessment).

CHANGELOG.md Show resolved Hide resolved
README.md Show resolved Hide resolved
pyuvdata/parameter.py Outdated Show resolved Hide resolved
@@ -293,6 +293,7 @@ def _write_ms_antenna(self, filepath):
ant_ref_frame = "ITRF"
else:
ant_ref_frame = self._telescope_location.frame.upper()
# TODO: ask Karto what the best way is to put in the lunar ellipsoid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ec3a52cd8352fe8569d00c3c22993303

pyuvdata/parameter.py Show resolved Hide resolved
pyuvdata/utils.py Outdated Show resolved Hide resolved
@kartographer
Copy link
Contributor

@bhazelton -- note you may need to clean up some of the CI actions here related to Python 3.9 to get past the testing checks...

Copy link
Member

@mkolopanis mkolopanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me so far as well. My only comment is dropping python 3.8 and 3.9 is big and we might want a version to make a clean cut.

README.md Show resolved Hide resolved
@bhazelton bhazelton merged commit 9141437 into main Mar 25, 2024
44 checks passed
@bhazelton bhazelton deleted the handle_lunar_ellipsoids branch March 25, 2024 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants