pyuvdata
pyuvdata copied to clipboard
Deprecate old phase attributes, use the new ones under the hood
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.
Codecov Report
Merging #1170 (15c7f94) into main (94ffaa1) will decrease coverage by
0.02%
. The diff coverage is99.15%
.
Additional details and impacted files
@@ 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.
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 @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.