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.

Sign up for GitHub

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

Jump to bottom

to_zarr(append_dim='dim0') doesn't need mode='a' #3123

Merged
merged 6 commits into from
Jul 29, 2019
Merged

to_zarr(append_dim='dim0') doesn't need mode='a' #3123

merged 6 commits into from
Jul 29, 2019

Conversation

davidbrochart
Copy link
Contributor

@davidbrochart davidbrochart commented Jul 14, 2019 •
edited
Loading

shoyer
shoyer reviewed Jul 16, 2019
View reviewed changes
xarray/backends/api.py Outdated
@@ -1118,7 +1118,8 @@ def to_zarr(dataset, store=None, mode='w-', synchronizer=None, group=None,
_validate_dataset_names(dataset)
_validate_attrs(dataset)

if mode == "a":
if (mode == 'a') or (append_dim is not None):
mode = 'a'
Copy link
Member

Choose a reason for hiding this comment

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

Normally the preferred way to handle cases like this (intellligent default argument values) is to make them default to None. Thus, if you write mode=‘w’ and append_dim=‘time’, you would get an error message rather than having your choice of mode overwritten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @shoyer, I've made mode default to None, which allows for checking inconsistency if append_dim is set and mode!='a' (I added a test for this). If mode and append_dim are not set, mode is internally set to w-, which is equivalent to the previous behavior.

@codecov
Copy link

codecov bot commented Jul 16, 2019 •
edited
Loading

Codecov Report

Merging #3123 into master will decrease coverage by 26.59%.
The diff coverage is 11.11%.

@@            Coverage Diff            @@
##           master   #3123      +/-   ##
=========================================
- Coverage      96%   69.4%   -26.6%     
=========================================
  Files          63      63              
  Lines       12777   12806      +29     
=========================================
- Hits        12266    8888    -3378     
- Misses        511    3918    +3407

@pep8speaks
Copy link

pep8speaks commented Jul 16, 2019 •
edited
Loading

Hello @davidbrochart! 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-07-16 08:13:10 UTC

@davidbrochart
Copy link
Contributor Author

@shoyer do you think this PR can be merged?

@shoyer shoyer merged commit dfa97da into pydata:master Jul 29, 2019
@shoyer
Copy link
Member

shoyer commented Jul 29, 2019

thanks!

@davidbrochart davidbrochart deleted the append_zarr branch July 29, 2019 18:49
dcherian added a commit to yohai/xarray that referenced this pull request Aug 3, 2019
@dcherian
Merge branch 'master' into yohai-ds_scatter
318abaa 
* master: (68 commits)
  enable sphinx.ext.napoleon (pydata#3180)
  remove type annotations from autodoc method signatures (pydata#3179)
  Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object (pydata#3095)
  Fix distributed.Client.compute applied to DataArray (pydata#3173)
  More annotations in Dataset (pydata#3112)
  Hotfix for case of combining identical non-monotonic coords (pydata#3151)
  changed url for rasterio network test (pydata#3162)
  to_zarr(append_dim='dim0') doesn't need mode='a' (pydata#3123)
  BUG: fix+test groupby on empty DataArray raises StopIteration (pydata#3156)
  Temporarily remove pynio from py36 CI build (pydata#3157)
  missing 'about' field (pydata#3146)
  Fix h5py version printing (pydata#3145)
  Remove the matplotlib=3.0 constraint from py36.yml (pydata#3143)
  disable codecov comments (pydata#3140)
  Merge broadcast_like docstrings, analyze implementation problem (pydata#3130)
  Update whats-new for pydata#3125 and pydata#2334 (pydata#3135)
  Fix tests on big-endian systems (pydata#3125)
  XFAIL tests failing on ARM (pydata#2334)
  Add broadcast_like. (pydata#3086)
  Better docs and errors about expand_dims() view (pydata#3114)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shoyer shoyer shoyer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Append to zarr redundant with mode='a'
3 participants
@davidbrochart @pep8speaks @shoyer