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 zarr append with groups #3610

Merged
merged 4 commits into from
Mar 2, 2020

Conversation

niowniow
Copy link
Contributor

@niowniow niowniow commented Dec 11, 2019

Fixes the issue that xarray.core.dataset.Dataset.to_zarr produced errors when using append_dim and group simultaneously. The issue was that during appending, the zarr store was opened without the group information.

@dcherian dcherian requested a review from rabernat December 11, 2019 15:38
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks @niowniow.

This looks OK to me but I don't know the zarr code.

doc/whats-new.rst Outdated Show resolved Hide resolved
doc/whats-new.rst Outdated Show resolved Hide resolved
doc/whats-new.rst Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor

dcherian commented Jan 9, 2020

@niowniow can you merge master please? The diff is huge right now

@niowniow
Copy link
Contributor Author

I tried to merge with master yesterday and did it again today. There might be a problem with the way I tried to fix the whats-new.rst issues. It might be faster if I open a new pull request and re-insert the changes!?

@keewis
Copy link
Collaborator

keewis commented Jan 10, 2020

If you don't mind I'll try to fix that for you.

@niowniow
Copy link
Contributor Author

I don't mind, thanks :)

@keewis
Copy link
Collaborator

keewis commented Jan 10, 2020

I cherry-picked your commits, but I'm not sure I included all the changes. Could you check that?

Also, I can't seem to figure out how to push to this PR, so you will have to do that instead:

git checkout fix_zarr_append_with_groups
# backup branch
git branch backup_fix_zarr_append_with_groups
# delete branch
git checkout master
git branch -D fix_zarr_append_with_groups
# checkout fixed branch
git checkout --track origin/fix_zarr_append_with_groups
# force push
git push -f <fork-remote> fix_zarr_append_with_groups

@niowniow niowniow force-pushed the fix_zarr_append_with_groups branch from daa111b to 2152b42 Compare January 10, 2020 14:32
@niowniow
Copy link
Contributor Author

Okay, this looks better. Thanks for the help! I had to try it again myself, because you cherry-picked some commits I accidentally merged into this branch (non-related to this fix).
Could you check if this is good now?

@keewis
Copy link
Collaborator

keewis commented Jan 10, 2020

Looks good to me. However, I don't know much about backends so I'll let someone else review the code.

@dcherian dcherian mentioned this pull request Jan 17, 2020
11 tasks
Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Sorry for my very slow review of this. I just looked over the PR and found zero issues. Thanks for your patience.

@rabernat
Copy link
Contributor

@dcherian - I assume these test failures (py38-upstream-dev and docs) are acceptable? They seem unrelated to the PR.

If so, please merge.

@keewis
Copy link
Collaborator

keewis commented Feb 26, 2020

the upstream-dev failure is #3751, but although there should be no change here that would trigger it, the docs failure (referencing api using :ref: instead of :doc:) does not seem to happen with other PRs (I also can't reproduce it locally).

@rabernat
Copy link
Contributor

the docs failure (referencing api using :ref: instead of :doc:) does not seem to happen with other PRs (I also can't reproduce it locally

I can't find that message here: https://dev.azure.com/xarray/xarray/_build/results?buildId=2214&view=logs&jobId=7e620c85-24a8-5ffa-8b1f-642bc9b1fc36&j=7e620c85-24a8-5ffa-8b1f-642bc9b1fc36&t=68484831-0a19-5145-bfe9-6309e5f7691d

To me it looks like the doc build failed due to two warnings: failure to fetch the pandas intersphinx obj and a sphinx deprecation warning about scripts.

@keewis
Copy link
Collaborator

keewis commented Feb 26, 2020

it's here:

/home/vsts/work/1/s/doc/faq.rst:239: WARNING: undefined label: api (if the link has no caption the label must precede a section header)

not sure if the intersphinx warning also makes the build fail, but that could be fixed by rerunning. The deprecation warning is not a problem.

@dcherian
Copy link
Contributor

dcherian commented Mar 2, 2020

All tests are green. Thanks @niowniow. I see this is your first contribution. Welcome!

@dcherian dcherian merged commit 8512b7b into pydata:master Mar 2, 2020
dcherian added a commit to ej81/xarray that referenced this pull request Mar 14, 2020
…e-size

* upstream/master: (24 commits)
  Fix alignment with join="override" when some dims are unindexed (pydata#3839)
  Fix CFTimeIndex-related errors stemming from updates in pandas (pydata#3764)
  Doctests fixes (pydata#3846)
  add xpublish to related projects (pydata#3850)
  update installation instruction (pydata#3849)
  Pint support for top-level functions (pydata#3611)
  un-xfail tests that append to netCDF files with scipy (pydata#3805)
  remove panel conversion (pydata#3845)
  Add nxarray to related-projects.rst (pydata#3848)
  Implement skipna kwarg in xr.quantile (pydata#3844)
  Allow `where` to receive a callable (pydata#3827)
  update macos image (pydata#3838)
  Label "Installed Versions" item in Issue template (pydata#3832)
  Add note on diff's n differing from pandas (pydata#3822)
  DOC: Add rioxarray and other external examples (pydata#3757)
  Use stable RTD image.
  removed mention that 'dims' are inferred from 'coords'-dict when omit… (pydata#3821)
  Coarsen keep attrs 3376 (pydata#3801)
  Turn on html repr by default (pydata#3812)
  Fix zarr append with groups (pydata#3610)
  ...
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.

Dataset.to_zarr() with mode='a' does not work with groups
4 participants