Skip to content

Added validation of attrs before saving to netCDF files #991

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

Merged
merged 5 commits into from
Sep 2, 2016

Conversation

robintw
Copy link
Contributor

@robintw robintw commented Aug 28, 2016

This allows us to give nice errors if users try to save a Dataset with
attr values that can't be written to a netCDF file.

Fixes #911.

I've added tests to test_backends.py as I can't see a better place to put them. I've also made the tests fairly extensive, but also used some helper functions to stop too much repetition. Please let me know if any of this doesn't fit within the xarray style.

This allows us to give nice errors if users try to save a Dataset with
attr values that can't be written to a netCDF file.

Fixes pydata#911
check_attr(k, v)

# Check attrs on each variable within the dataset
for var_name in dataset.data_vars:
Copy link
Member

@shoyer shoyer Aug 28, 2016

Choose a reason for hiding this comment

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

Use dataset.variables (a dict of xarray.Variable objects) instead of separately checking data vars and coords

@robintw
Copy link
Contributor Author

robintw commented Aug 28, 2016

Thanks, I'll deal with these modifications.

Also, I'm seeing errors from TravisCI saying ValueError: cannot read or write netCDF files without netCDF4-python or scipy installed - what do I need to do to make sure these tests work on Travis? (They pass fine on my local machine)

@shoyer
Copy link
Member

shoyer commented Aug 28, 2016

Take a look at the other netcdf tests -- they all use a decorator like
@requires_netCDF4, either on the class or the method. That ensures they are
skipped if required libraries for the functionality are not installed.
On Sun, Aug 28, 2016 at 2:43 PM Robin Wilson [email protected]
wrote:

Thanks, I'll deal with these modifications.

Also, I'm seeing errors from TravisCI saying ValueError: cannot read or
write netCDF files without netCDF4-python or scipy installed - what do I
need to do to make sure these tests work on Travis? (They pass fine on my
local machine)


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#991 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABKS1qrd1mmelzaoL4omWV_TTLwAMeUSks5qkgEWgaJpZM4Ju5fU
.

@robintw
Copy link
Contributor Author

robintw commented Aug 31, 2016

I think I've dealt with all of these.

Refactoring the tests has made them a bit cleaner and safer due to not using mutation - however I still have quite a few repeated lines of code. I've also combined them all into one test with lots of asserts - as otherwise I couldn't keep the helper functions within the test without repeating them loads of times.

Does anything need adding to the docs for this? I'm assuming not as it's just making the code do more checking, it's not adding a feature as such?

@robintw
Copy link
Contributor Author

robintw commented Sep 1, 2016

The Travis builds seem to have all passed except for py27-cdat+pynio which has failed with a conda error that seems to be unrelated to these changes (Error: HTTPError: 403 Client Error: Forbidden for url)

check_attr(k, v)

# Check attrs on each variable within the dataset
for var_name in dataset.variables:
Copy link
Member

Choose a reason for hiding this comment

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

dataset.variables is a dictionary. So you can write:

for variable in dataset.variables.values():
    for k, v in variable.attrs.items():
        check_attr(k, v)

@shoyer
Copy link
Member

shoyer commented Sep 2, 2016

This is almost there now.

I agree that we don't need to update the docs, but please do make note of it in "What's New".

@shoyer shoyer merged commit c80aa82 into pydata:master Sep 2, 2016
@shoyer
Copy link
Member

shoyer commented Sep 2, 2016

thanks!

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.

2 participants