-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Contiguous store with unlim dim bug fix #2941
Conversation
Hello @jmccreight! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-06-04 16:16:07 UTC |
I guess the additional documentation would go here. http://xarray.pydata.org/en/stable/io.html#writing-encoded-data |
I should add that changing the encoding on the variable itself was not "an easy detail". The basic thing I tried was unsuccessful, so if this is desired some more work is needed. |
Thanks @jmccreight. That was quick. I think we can do without the warning but we should wait till someone else chimes in. Otherwise, this looks right to me but I don't know this bit of the code too well. |
There were easy patterns to follow for this, so I just went for it. |
The failures seem unrelated. can you merge master and remove the 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.
LGTM. Just some minor changes.
I'll merge in a couple of days if no one else has comments.
xarray/backends/netCDF4_.py
Outdated
@@ -210,6 +210,10 @@ def _extract_nc4_variable_encoding(variable, raise_on_invalid=False, | |||
if chunks_too_big or changed_shape: | |||
del encoding['chunksizes'] | |||
|
|||
var_has_unlim_dim = any(dim in unlimited_dims for dim in variable.dims) | |||
if var_has_unlim_dim and 'contiguous' in encoding.keys(): |
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 should also check for not raise_on_invalid
, like what we do above for chunksizes
. Otherwise, if a user specified an invalid encoding by hand we would silently ignore their 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.
@shoyer I think this is what you mean.
if not raise_on_invalid and var_has_unlim_dim and 'contiguous' in encoding.keys():
del encoding['contiguous']
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.
Thanks @jmccreight . We prefer using ()
to wrap long lines instead of using a \
. I've made the fix.
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.
Thanks. I was, in fact, very uncertain about the preferred approach.
=). Thanks @jmccreight. Merging. |
whats-new.rst
for all changes andapi.rst
for new APINot sure this needs documentation else where...