Pol convention

[steven-murray]

trafficstars

Description

This adds a new parameter pol_convention to both UVData and UVCal, as well as uvcalibrate which applies it from one to the other. The parameter is not required, but a warning is issued if it is not specified on data that is calibrated, and an error is raised if it is specified when the data is not calibrated.

In uvcalibrate, I am forcing the parameter to be set (either inherited from the UVCal or set directly as a keyword to the function).

Motivation and Context

The primary motivation is to keep track of this convention in data, so that the combination of calibration and power-spectrum estimation can be done consistently without "external knowledge". This is something useful for HERA at the moment.

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

[steven-murray]

Thanks @bhazelton -- I've fixed up the things you suggested, and also added the read/write to uvh5. I don't know enough about fits/ms to help there, so if you could manage that part, I'd be very grateful. I added a test for uvdata to test that warnings/errors are raised appropriately. My guess is that read/write will be automatically covered in round-trip tests.

[steven-murray]

However, there might be a lot of new warnings now with old file types.

[jsdillon]

Is this branch at the point where I can use it to start writing files with the pol convention attribute, or should I hold off?

[steven-murray]

@bhazelton we need to decide how we want to fix the breaking tests:

1. Update all the lists of expected warnings to include one more
2. Update (most of) the data files so the warnings aren't raised.
3. something else?

[bhazelton]

@bhazelton we need to decide how we want to fix the breaking tests:

1. Update all the lists of expected warnings to include one more
2. Update (most of) the data files so the warnings aren't raised.
3. something else?

I can accept either 1 or 2, although maybe we need a wider discussion about whether we want to warn about this every time we call check, especially on UVData because this is not something that exists on any of the existing formats we support, so it feels a little presumptuous to yell at all our users about it all the time. Maybe we can instead warn only in the uvcalibrate method if it's not set and then advertise the existence of this new attribute in the tutorial and docs?

[bhazelton]

I have a question: Should we error in UVCal.check if the pol_convention is set but the gain_scale isn’t? I feel like setting the pol_convention without the gain_scale isn’t very meaningful. This is already partially handled in uvcalibrate where the pol_convention isn't used if gain_scale isn't set, but I'm wondering about adding this to the UVCal check.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.93%. Comparing base (3b412bc) to head (f66efe3). Report is 127 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1455   +/-   ##
=======================================
  Coverage   99.92%   99.93%           
=======================================
  Files          61       61           
  Lines       21380    21461   +81     
=======================================
+ Hits        21365    21446   +81     
  Misses         15       15           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jsdillon]

I think a warning is probably sufficient, but I wouldn't necessarily be opposed to an error in that case.

[steven-murray]

I agree with @jsdillon that a warning is the right balance here.

[bhazelton]

I added the warning and updated the docstrings a bit.

[jsdillon]

Thanks all!

You planning a new minor version release on pypi?