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

Version 3 -- Endgame #1437

Merged
merged 25 commits into from
May 24, 2024
Merged

Version 3 -- Endgame #1437

merged 25 commits into from
May 24, 2024

Conversation

kartographer
Copy link
Contributor

@kartographer kartographer commented May 16, 2024

"I am inevitable" -- V3 of pyuvdata

Description

Implements the final set of changes for pyuvdata version 3.0, primarily:

  • Dropping support for "current array shapes" (the old spw-axis -- which was forced to always be 1 -- has been removed from attributes that have it)
  • Requiring flexible spectral windows be used in UVData and UVCal` objects (the latter only when a frequency axis is present, i.e., non-wideband solutions)
  • Dropping support for the "old" phasing parameters (which were deprecated when phase_center_catalog became a standard parameter).

Relatedly, the use_future_array_shapes method has been deprecated, and is planned to be removed in v3.2.

Note that while nearly all the required work on this branch is complete, I haven't updated the CHANGELOG yet, mostly in anticipation of (perhaps unlikely) significant changes that may be requested. Also, this PR will follow #1422, and will be moved to "ready" once that PR is successfully merged into prep_v3.0.

Motivation and Context

This PR is meant to be the final PR to close out V3 updates, and as such should be the final one merged into prep_v3.0 before that branch is ready or merge into main. And I for one am ready to overthrow our vestigal spw-axis overlords.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Breaking change checklist:

  • I have updated the docstrings associated with my change using the numpy docstring format.
  • I have updated the tutorial to reflect my changes (if appropriate).
  • My change includes backwards compatibility and deprecation warnings (if possible).
  • I have added tests to cover my changes.
  • All new and existing tests pass.
  • I have updated the CHANGELOG.

Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.92%. Comparing base (3f6e830) to head (38060d2).

Additional details and impacted files
@@              Coverage Diff              @@
##           prep_v3.0    #1437      +/-   ##
=============================================
- Coverage      99.93%   99.92%   -0.01%     
=============================================
  Files             41       41              
  Lines          22702    21138    -1564     
=============================================
- Hits           22687    21123    -1564     
  Misses            15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bhazelton bhazelton left a comment

Choose a reason for hiding this comment

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

I took a medium-deep look (I did not read through all the changes in all the test files) and I think it's looking good, I just have a few small comments. I note that there are some external tests failing that I need to look into.

pyuvdata/uvbeam/uvbeam.py Outdated Show resolved Hide resolved
pyuvdata/uvdata/uvdata.py Show resolved Hide resolved
pyuvdata/uvdata/uvfits.py Show resolved Hide resolved
pyuvdata/uvcal/calfits.py Show resolved Hide resolved
@bhazelton
Copy link
Member

bhazelton commented May 20, 2024

It looks like the biggest problem on pyuvsim is that there's no future_array_shapes attribute on UVData objects. Maybe we should leave that around for a little longer? we can deprecate it with a __getattr__ on UVBase that just warns that it's going away and returns True?

It looks like the hera_qm tests are running into this same problem, mostly on UVFlag objects.

@kartographer
Copy link
Contributor Author

@bhazelton -- okay, I added back in future_array_shapes into UVBase with a similar deprecation warning to what the telescope attributes are throwing.

@kartographer
Copy link
Contributor Author

@bhazelton -- note that there are now new tests that are failing in those external pipelines b/c of the new warnings. Looks like the failures are at least a bit more contained, but there may need to be a bit more clean-up in those repos.

pyuvdata/uvbase.py Outdated Show resolved Hide resolved
@kartographer kartographer marked this pull request as ready for review May 22, 2024 00:40
@bhazelton
Copy link
Member

It looks like we lost coverage of one line in miriad.py that was covered before (line 1579 where fix_phase is called). You can see the line in the Indirect Changes tab on the codecov report.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.93%. Comparing base (3f6e830) to head (c167883).

Additional details and impacted files
@@              Coverage Diff              @@
##           prep_v3.0    #1437      +/-   ##
=============================================
- Coverage      99.93%   99.93%   -0.01%     
=============================================
  Files             41       41              
  Lines          22702    21138    -1564     
=============================================
- Hits           22687    21124    -1563     
+ Misses            15       14       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bhazelton bhazelton left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!!

@bhazelton bhazelton merged commit cfc431f into prep_v3.0 May 24, 2024
43 of 46 checks passed
@bhazelton bhazelton deleted the v3_endgame branch May 24, 2024 03:14
@kartographer kartographer mentioned this pull request Jun 5, 2024
5 tasks
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.

3 participants