pyuvdata icon indicating copy to clipboard operation
pyuvdata copied to clipboard

Deprecate old phase attributes, use the new ones under the hood

Open bhazelton opened this issue 2 years ago • 2 comments

Description

Deprecate the old phase attributes in favor of the phase_center_catalog that can support more types of phase centers and multiple phase centers. This is done by always using phase_center_catalog under the hood and using __getattribute__ to handle requests for the old phase attributes.

Other things included in this PR:

  • add support for selecting on phase_center_ids
  • fix bug in partial read with lsts or lst_range parameters

Motivation and Context

closes #1094

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [x] 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.

Breaking change checklist:

  • [x] 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).
  • [x] My change includes backwards compatibility and deprecation warnings (if possible).
  • [x] I have added tests to cover my changes.
  • [x] All new and existing tests pass.
  • [ ] I have updated the CHANGELOG.

bhazelton avatar May 19 '22 15:05 bhazelton

Codecov Report

Merging #1170 (15c7f94) into main (94ffaa1) will decrease coverage by 0.02%. The diff coverage is 99.15%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1170      +/-   ##
==========================================
- Coverage   99.93%   99.91%   -0.03%     
==========================================
  Files          33       33              
  Lines       18303    18573     +270     
==========================================
+ Hits        18292    18557     +265     
- Misses         11       16       +5     
Impacted Files Coverage Δ
pyuvdata/uvbeam/beamfits.py 100.00% <ø> (ø)
pyuvdata/uvcal/calfits.py 100.00% <ø> (ø)
pyuvdata/uvcal/uvcal.py 100.00% <ø> (ø)
pyuvdata/uvdata/uvdata.py 100.00% <ø> (ø)
pyuvdata/uvdata/miriad.py 98.45% <97.39%> (-0.29%) :arrow_down:
pyuvdata/uvdata/ms.py 99.88% <99.25%> (-0.12%) :arrow_down:
pyuvdata/parameter.py 100.00% <100.00%> (ø)
pyuvdata/utils.py 100.00% <100.00%> (ø)
pyuvdata/uvbase.py 100.00% <100.00%> (ø)
pyuvdata/uvbeam/cst_beam.py 100.00% <100.00%> (ø)
... and 10 more

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 94ffaa1...15c7f94. Read the comment docs.

codecov[bot] avatar May 19 '22 15:05 codecov[bot]

I had one other, slightly sneakier, thought after having a night to sleep on it and think about it further. Rather than continuing to support the two cases of "multi-pc" versus "single pc", might it be worth instead making everything a multi-phase-center dataset under the hood, where the deprecated attributes point to either the one source in the catalog (if Nphase=1) or an appropriately derived value? User-side, the object would behave very similarly, but all of the extra code that's needed could just be limited to __getattribute__. It would be a fair bit more work upfront, although it might end up being a bit easier to maintain. If there's interest, I can spell it out a bit further during the next telecon.

kartographer avatar Jul 31 '22 22:07 kartographer

@kartographer @mkolopanis I'm not sure how to make tests to cover the missed lines in miriad.py and ms.py. They seems to require making files through the lower level API. I've played around a little with those and I got some more coverage but haven't been able to figure out how to cover the remaining uncovered lines.

bhazelton avatar Nov 30 '22 00:11 bhazelton