Add '_FillValue' to set of valid_encodings for netCDF4 backend #1869
Conversation
|
Oops! Yes, this is was definitely a bug. Can you add a unit test for this, so we don't have a regression? Take a look at the test |
Add additional tests to prevent future regression of setting _FillValue to None when using the encoding kwarg in to_netcdf.
|
Unit tests added in 9210dfa. The first and the third added tests use the encoding kwarg and fail without b8fd86d applied and pass with b8fd86d applied, so they should catch a regression. The second added test passes both before and after b8fd86d and was added for completeness as it pairs with the original test_explicitly_omit_fill_value, but for a coordinate. I still think nothing needs to change in the documentation as this now follows the intent of the design. I will leave it to you @shoyer or your delegate to address whether anything needs to be stated in the changelog / what's new. |
|
Wait to merge... I'm still getting failed tests using the scipy backend |
Remove copy/paste line that shouldn't have been in a test. Add additional asserts. Fix indentation.
|
Now the tests pass (see comment beside the checkbox) Honestly, I'm not happy with where I put the fix in order to get the tests to pass when using the scipy backend by adding an additional test, but I didn't know where else to put the test. The test to see if the variable would be checked in prepare_variables came before the encoding could be managed in encode_cf_variable in conventions.py The alternative would be to remove the changes to scipy_.py and just have the test run only for the other engine backends and leave fixing the scipy backend for someone more familiar with it for another issue. |
|
@shoyer, do you have an opinion on whether or not to include the scipy fix or leave it out of this PR? |
|
We don't need any additional documentation, but we should note this bug fix in "What's new" |
| @@ -188,8 +188,9 @@ def encode_variable(self, variable): | |||
| def prepare_variable(self, name, variable, check_encoding=False, | |||
| unlimited_dims=None): | |||
| if check_encoding and variable.encoding: | |||
| raise ValueError('unexpected encoding for scipy backend: %r' | |||
| % list(variable.encoding)) | |||
| if variable.encoding != {'_FillValue': None}: | |||
There was a problem hiding this comment.
Yeah, this is a little awkward.
I think this is probably OK (backend classes already need to know about encodings), but it would be good to think about why we don't already have validation for encodings with scipy. We should probably be raising an error if somebody attempts an encoding like {'zlib': True} but right now it looks like we don't do that.
|
@shoyer, I think everything is in order to apply this bug fix. |
ghost
mentioned this pull request
|
@shoyer - this looks good to me. At some point, we should formalize how backends validate their encodings but I don't think that needs to happen here. |
|
Yes, thanks @czr137 ! |
whats-new.rstfor all changes andapi.rstfor new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)