Skip to content
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

Enable cross-pol power spectra *and* Python 3 support #189

Merged
merged 27 commits into from
Mar 7, 2019
Merged

Conversation

philbull
Copy link
Collaborator

@philbull philbull commented Mar 3, 2019

This PR enables cross-polarisation support, subject to the limitation that the beam scalar can't be computed for cross-pol yet. (In other words, this PR only allows unnormalised cross-pol spectra to be computed.)

The main change in the PR is to store polarisation pairs inside UVPSpec, instead of individual polarisations. Pol-pairs can be specified as tuples, e.g. ('xx', 'xx'), or as specially-constructed integers (there are helper functions to deal with these). Individual polarisations (e.g. 'xx') are no longer accepted.

I also got a bit carried away and enabled Python 3 support in this PR. This required more extensive changes than first expected, as functions like map() and reduce() have been deprecated (now replaced with list comprehensions and np.reduce), some string handling has changed (especially annoying for HDF5), and methods like dict.keys() now return iterators instead of lists, and so can't be indexed directly (fixed by calling list() on the iterators).

 * Bugfixes in tests to make cross-pol work
 * Replace deprecated Py2 functions, e.g. map(), reduce()
 * Adapt to new behaviour of dict.keys() etc. which return
   iterators instead of lists now
@ghost ghost assigned philbull Mar 3, 2019
@ghost ghost added the in progress label Mar 3, 2019
@philbull philbull requested review from nkern and aewallwi March 3, 2019 12:45
@coveralls
Copy link

coveralls commented Mar 3, 2019

Coverage Status

Coverage increased (+0.2%) to 96.654% when pulling 39d526a on enable_xpol into d2de2ff on master.

@philbull
Copy link
Collaborator Author

philbull commented Mar 3, 2019

The tests are only failing because of a weird bug with the print statement in version.py in Python 2.x.

hera_pspec/uvpspec.py Outdated Show resolved Hide resolved
hera_pspec/uvpspec_utils.py Outdated Show resolved Hide resolved
@philbull
Copy link
Collaborator Author

philbull commented Mar 5, 2019

OK, I managed to fix the test failure (it was due to a change in io.StringIO behaviour between Py2 and Py3, used by the test suite). I also made some improvements in test coverage and caught a bug, so coveralls passes now too.

hera_pspec/pspecbeam.py Outdated Show resolved Hide resolved
hera_pspec/pspecbeam.py Outdated Show resolved Hide resolved
hera_pspec/pspecdata.py Outdated Show resolved Hide resolved
hera_pspec/pspecdata.py Outdated Show resolved Hide resolved
hera_pspec/uvpspec.py Outdated Show resolved Hide resolved
hera_pspec/uvpspec.py Outdated Show resolved Hide resolved
@nkern
Copy link
Member

nkern commented Mar 5, 2019

@philbull just for posterity: on the hera_pspec telecon 3/5/18 I think we decided to punt on cross-visibility-polarization scalars, and that cross-visibility-pol jacknives should be done in "data units" rather than "cosmo units"--so no scalar.

Also, I take back my support for interpreting polpairs = ['XX'] as polpairs = [('XX', 'XX')], because I think that can easily lead to confusion over 'XX' being a "polarization-pair", which it isn't in our definition, although it is technically a "feed-polarization-pair". Up to you @philbull what you want to do about it though..

@nkern
Copy link
Member

nkern commented Mar 5, 2019

@philbull so I've finished my skim of the PR. it looks good to me with minor requests for revision. ping me when you are done w/ edits and i'll approve!

@philbull
Copy link
Collaborator Author

philbull commented Mar 6, 2019

Thanks Nick! I've made all the changes you suggested, and improved test coverage slightly to get that coveted green checkmark.

Copy link
Member

@nkern nkern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @philbull for getting us py3k supported and for the cross-visibility-pol functionality!

@philbull philbull merged commit 0bee9d2 into master Mar 7, 2019
@ghost ghost removed the in progress label Mar 7, 2019
@philbull philbull deleted the enable_xpol branch March 7, 2019 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants