Pol convention #1455

steven-murray · 2024-07-05T12:45:19Z

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)
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:

I have read the contribution guide.
My code follows the code style of this project.

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.

bhazelton

This is a good start but we need ways to round trip this new parameter through the various file formats for UVCal and UVData. I'm happy to help on that front if you need.

src/pyuvdata/utils/uvcalibrate.py

steven-murray · 2024-07-08T09:26:16Z

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 · 2024-07-08T09:26:38Z

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

jsdillon · 2024-07-08T19:58:54Z

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 · 2024-07-09T01:25:28Z

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

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

bhazelton · 2024-07-10T00:41:14Z

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

Update all the lists of expected warnings to include one more

Update (most of) the data files so the warnings aren't raised.

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?

…on that are not none

kartographer

I think overall the code is looking in pretty good shape, and have nothing in terms of detailed comments to offer aside from highlighting that the doctest is still failing.

As a more general comment, I do think it would be good to put a longer explanation into the tutorials about this, since I can easily see where this might trip someone up. In particular, I think most of the higher frequency interferometers (e.g., VLA, ALMA, SMA, ATCA, ATA) take the "avg" approach, so it may be good to be explicit about what's typical/where "sum" is definitely used (since I think its the less common option of the two).

bhazelton · 2024-07-15T19:24:35Z

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 · 2024-07-15T19:34:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

jsdillon · 2024-07-15T19:50:35Z

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

steven-murray · 2024-07-16T10:20:39Z

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

bhazelton · 2024-07-16T18:39:01Z

I added the warning and updated the docstrings a bit.

src/pyuvdata/utils/uvcalibrate.py

bhazelton

Thanks for the new docs, they are great. Just a few comments.

Still working on figuring out the circleci issue...

docs/uvcal_tutorial.rst

src/pyuvdata/utils/uvcalibrate.py

tests/utils/test_uvcalibrate.py

bhazelton · 2024-07-19T20:02:46Z

src/pyuvdata/utils/uvcalibrate.py

+def _apply_pol_convention_corrections(
+    uvdata, undo, uvd_pol_convention, uvc_pol_convention
+):
+    if uvd_pol_convention != uvc_pol_convention:
+        correction = np.ones(uvdata.Npols)
+        correction[uvdata.polarization_array > 0] *= 2
+        correction[uvdata.polarization_array < 0] /= 2
+
+        if (undo and uvd_pol_convention == "sum") or (
+            not undo and uvd_pol_convention == "avg"
+        ):
+            correction = 1 / correction
+
+        uvdata.data_array *= correction
+
+    uvdata.pol_convention = None if undo else uvd_pol_convention


This is pretty hard to parse and be confident in, but I think your test shows that it's working properly, at least for linear pols. I don't see a test on Stokes visibilities (i.e. when uvdata.polarization_array > 0). Is there a test for that case?

Also, please move this to above uvcalibrate so it's defined before it's called.

OK cool. I moved the function, wrote a docstring for it to hopefully make the logic more clear, and also added tests for circular polarizations. However, in doing this I found that it seems that it is not possible to calibrate/decalibrate uvdata objects with stokes polarizations (it complains that the polarizations can't be found on the UVCal object).

Is this to be expected?

I don't really know about a case where stokes calibration solutions make sense, but I feel like @kartographer ran into it when he was implementing MS cal read/write?

So this is definitely a corner-case (that could be wrapped with a "pyuvdata does not support... if you file an issue..."), but the most common case I ran into was visibilities recorded as (pseudo-)Stokes I, which can have gains tables corresponding also to (pseudo-)Stokes I. Looking at the UVCal class though, we don't currently allow for Jones > 0, which is maybe what's causing the issue here.

This got raised in the MS read/write mostly on the visibility side, since the MS format has a FEED table that I had to match to the data, hence why those pseudo Stokes values are supported there.

src/pyuvdata/utils/uvcalibrate.py

kartographer

Just a few additional comments...

kartographer · 2024-07-23T11:42:27Z

docs/uvcal_tutorial.rst

+``I = (XX + YY)/2`` (the ``avg`` convention) or ``I = XX + YY`` (the ``sum``
+convention). This choice is generally encoded in the sky model to which the visibilities
+are calibrated. Different tools and simulators make different choices, generally following
+a standard choice for the field. In ``pyuvdata`` either of these choices are OK, but the


Different tools and simulators make different choices, generally following a standard choice for the field.

I think we need to be more explicit here about which tools do what, especially if we are going to recommend that users always specify this parameter. In particular, I believe the tasks in CASA (e.g., tclean) and MIRIAD, along with WSClean, all assume the avg convention. I actually am not sure of what uses the sum convention, though I expect we can gather that information.

I'm not very familiar with such tools, but I'll add that particular information.

FHD uses the sum convention and the various HERA packages are moving to it.

Cool. I added that as well.

kartographer · 2024-07-23T11:47:42Z

src/pyuvdata/utils/uvcalibrate.py

+    if uvc_pol_convention is None and uvcal.pol_convention is None:
+        warnings.warn(
+            message=(
+                "pol_convention is not specified on the UVCal object, and "


I don't know that I'd want this to error when the pol convention is not specified on either UVCal and UVData, at least not if we're treating pol_convention as an optional parameter. Why can't this just stay a warning?

Sorry @kartographer, I'm not following -- this is a warning?

@steven-murray -- sorry, I mean a general UserWarning, not a DeprecationWarning. I.e., this should alert the user that there's a potential for mismatch, but I don't think this should throw an error.

OK, I updated all the DeprecationWarning's to UserWarnings. Originally I had intended these to be DeprecationWarnings because we want to push people to use the keywords. However, it would be really annoying to have that warning if you really just don't care about the final units (like, you're just using calibration solutions to multiply your data by 2...)

kartographer

Looking good to me -- thanks @steven-murray!

jsdillon · 2024-07-23T21:01:46Z

Thanks all!

You planning a new minor version release on pypi?

steven-murray requested a review from bhazelton July 5, 2024 12:46

steven-murray self-assigned this Jul 5, 2024

steven-murray requested review from kartographer and jsdillon July 5, 2024 12:47

bhazelton reviewed Jul 5, 2024

View reviewed changes

src/pyuvdata/utils/uvcalibrate.py Outdated Show resolved Hide resolved

src/pyuvdata/utils/uvcalibrate.py Outdated Show resolved Hide resolved

bhazelton added Calibration UVData labels Jul 5, 2024

steven-murray force-pushed the pol-convention branch from 7256e91 to 614abd7 Compare July 8, 2024 09:11

steven-murray and others added 9 commits July 12, 2024 21:29

feat: add pol_convention parameter

d51a637

docs: add to changelog

17dae1f

fix: circular import

7d43ed6

fix: change behaviour of missin pol_convention

0feb93a

style: add type-hints to uvcalibrate

99f56a5

fix: add read/write of pol_convention to uvh5 and calh5

70dc32b

test: add basic test of polarization conventions for uvd

b244201

fix bug that causes error on reading files with polarization conventi…

13b0063

…on that are not none

remove warning for pol_convention in uvdata and uvcal

67c6d53

steven-murray force-pushed the pol-convention branch from 22713d7 to 8592e49 Compare July 12, 2024 20:38

steven-murray and others added 2 commits July 12, 2024 22:39

fix: add warning for missing gain_scale in uvcalibrate

8592e49

Fix tests

f2f8f1d

kartographer reviewed Jul 15, 2024

View reviewed changes

bhazelton added 3 commits July 15, 2024 11:59

Add read/write handling for pol_convention across file types

ef627df

Fix min_deps tests

4450b4a

Fix tutorial

161ca81

bhazelton added the HERA Things needed to support HERA development. label Jul 15, 2024

bhazelton added 2 commits July 16, 2024 10:41

Add warning for pol_convention set and gain_scale is not

c405532

Update the docstrings for pol_convention

738cc16

bhazelton reviewed Jul 16, 2024

View reviewed changes

src/pyuvdata/utils/uvcalibrate.py Outdated Show resolved Hide resolved

docs: update uvcal_tutorial

e8ade38

bhazelton reviewed Jul 17, 2024

View reviewed changes

docs/uvcal_tutorial.rst Outdated Show resolved Hide resolved

docs/uvcal_tutorial.rst Outdated Show resolved Hide resolved

docs/uvcal_tutorial.rst Show resolved Hide resolved

src/pyuvdata/utils/uvcalibrate.py Show resolved Hide resolved

jsdillon mentioned this pull request Jul 17, 2024

Implement psuedostokes that uses pol_convention appropriately HERA-Team/hera_pspec#403

Closed

docs: update docs to be more clear around pol_convention

631641f

adeliegorce mentioned this pull request Jul 18, 2024

Implement pol_convention in pstokes module HERA-Team/hera_pspec#404

Merged

steven-murray added 2 commits July 19, 2024 12:01

test: add tests for uvcalibrate with pol_conventions

37ad4bd

test: ignore warnings in pol conventions tests

e8610a6

bhazelton reviewed Jul 19, 2024

View reviewed changes

test: add tests for circular pols

14f0cca

bhazelton reviewed Jul 22, 2024

View reviewed changes

src/pyuvdata/utils/uvcalibrate.py Show resolved Hide resolved

bhazelton reviewed Jul 22, 2024

View reviewed changes

src/pyuvdata/utils/uvcalibrate.py Outdated Show resolved Hide resolved

bhazelton reviewed Jul 22, 2024

View reviewed changes

src/pyuvdata/utils/uvcalibrate.py Outdated Show resolved Hide resolved

fix: remove print statements

760e327

bhazelton requested a review from kartographer July 22, 2024 17:03

kartographer reviewed Jul 23, 2024

View reviewed changes

steven-murray added 2 commits July 23, 2024 15:48

ui: raise more informative error when uvd has stokes pols in uvcalibrate

9ca2714

fix: typo in any check

f66efe3

kartographer approved these changes Jul 23, 2024

View reviewed changes

bhazelton merged commit 6148ba9 into main Jul 23, 2024
46 checks passed

bhazelton deleted the pol-convention branch July 23, 2024 20:17

bhazelton mentioned this pull request Aug 13, 2024

Add new metadata to UVCal and UVData objects to track the polarization normalization #1441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pol convention #1455

Pol convention #1455

steven-murray commented Jul 5, 2024 •

edited

Loading

bhazelton left a comment

steven-murray commented Jul 8, 2024

steven-murray commented Jul 8, 2024

jsdillon commented Jul 8, 2024

steven-murray commented Jul 9, 2024

bhazelton commented Jul 10, 2024

kartographer left a comment

bhazelton commented Jul 15, 2024

codecov bot commented Jul 15, 2024 •

edited

Loading

jsdillon commented Jul 15, 2024

steven-murray commented Jul 16, 2024

bhazelton commented Jul 16, 2024

bhazelton left a comment

bhazelton Jul 19, 2024

steven-murray Jul 21, 2024

bhazelton Jul 22, 2024

kartographer Jul 23, 2024

kartographer left a comment

kartographer Jul 23, 2024

steven-murray Jul 23, 2024

bhazelton Jul 23, 2024

steven-murray Jul 23, 2024

kartographer Jul 23, 2024

steven-murray Jul 23, 2024

kartographer Jul 23, 2024

steven-murray Jul 23, 2024

kartographer left a comment

jsdillon commented Jul 23, 2024

Pol convention #1455

Pol convention #1455

Conversation

steven-murray commented Jul 5, 2024 • edited Loading

Description

Motivation and Context

Types of changes

Checklist:

bhazelton left a comment

Choose a reason for hiding this comment

steven-murray commented Jul 8, 2024

steven-murray commented Jul 8, 2024

jsdillon commented Jul 8, 2024

steven-murray commented Jul 9, 2024

bhazelton commented Jul 10, 2024

kartographer left a comment

Choose a reason for hiding this comment

bhazelton commented Jul 15, 2024

codecov bot commented Jul 15, 2024 • edited Loading

Codecov Report

jsdillon commented Jul 15, 2024

steven-murray commented Jul 16, 2024

bhazelton commented Jul 16, 2024

bhazelton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartographer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartographer left a comment

Choose a reason for hiding this comment

jsdillon commented Jul 23, 2024

steven-murray commented Jul 5, 2024 •

edited

Loading

codecov bot commented Jul 15, 2024 •

edited

Loading