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

Fix backwards compatibility bugs with UVPSpec data files #195

Merged
merged 7 commits into from
Mar 27, 2019

Conversation

philbull
Copy link
Collaborator

Fix two backwards compatibility bugs related to loading old HDF5 files into UVPSpec objects.

The first is related to HDF5 in Py3, which returns bytes instead of strings. A string comparison was failing as a result. All string attributes in UVPSpec files are now converted from bytes to strings.

The second is due to the recent change from pol_array to polpair_array in the UVPSpec data model. The old pol_array attribute is now detected and automatically converted to a polpair_array when a file is loaded. A UserWarning is shown when this happens.

@ghost ghost assigned philbull Mar 26, 2019
@ghost ghost added the in progress label Mar 26, 2019
@philbull philbull added bug and removed in progress labels Mar 26, 2019
@philbull
Copy link
Collaborator Author

This should fix issue #194.

@philbull philbull requested a review from nkern March 26, 2019 09:01
@ghost ghost added the in progress label Mar 26, 2019
@coveralls
Copy link

coveralls commented Mar 26, 2019

Coverage Status

Coverage decreased (-0.09%) to 96.516% when pulling ae0307e on uvpspec_backwards_compat into 0bee9d2 on master.

- source activate test-environment
- conda install -c conda-forge healpy aipy scikit-learn
- conda install -c conda-forge healpy aipy
Copy link
Member

@nkern nkern Mar 26, 2019

Choose a reason for hiding this comment

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

hera_qm and hera_cal, although they don't strictly require it, have some advanced functionality that relies on scikit-learn, so it'd be nice to keep this if possible. was something breaking with the installation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Nick. The reason from removing scikit-learn from this line is that it was pulling in an old version of numpy, which is now incompatible with pyuvdata (thus causing the tests to fail). It is installed via pip later on in the Travis script however (pip allows it to install with a more recent version of numpy).

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 good to me, but I'd like to keep scikit-learn installation if its not breaking anything

@plaplant
Copy link
Member

Thanks for the quick turnaround @philbull!

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

Successfully merging this pull request may close these issues.

None yet

4 participants