Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Pol convention #1455
Pol convention #1455
Changes from 17 commits
d51a637
17dae1f
7d43ed6
0feb93a
99f56a5
70dc32b
b244201
13b0063
67c6d53
8592e49
f2f8f1d
ef627df
4450b4a
161ca81
c405532
738cc16
e8ade38
631641f
37ad4bd
e8610a6
14f0cca
760e327
9ca2714
f66efe3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thesum
convention, though I expect we can gather that information.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with such tools, but I'll add that particular information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FHD uses the sum convention and the various HERA packages are moving to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I added that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @kartographer, I'm not following -- this is a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steven-murray -- sorry, I mean a general
UserWarning
, not aDeprecationWarning
. I.e., this should alert the user that there's a potential for mismatch, but I don't think this should throw an error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.