pangeo-data / rechunker

Disk-to-disk chunk transformation for chunked arrays.
https://rechunker.readthedocs.io/
MIT License
163 stars 25 forks source link

Traverse the source zarr store copying the groups and attributes into… #134

Closed flamingbear closed 1 year ago

flamingbear commented 1 year ago

:wave: hey there. I looked into this a bit for my work and wanted to leave this for someone in case they thought it was useful. In my work, I'm still using v0.5 and running this basic command after the rechunk happens to pick up all of the missing group metadata. Hope this is helpful. If I get some time I will try to update with some tests if this seems useful.

rabernat commented 1 year ago

Welcome Mike and thanks for your contribution! I agree this seems useful.

I don't understand why the test workflow did not run on this PR. 🤔

flamingbear commented 1 year ago

CI didn't kick off, because actions weren't enabled on my fork. I pushed a space change to run them.

rabernat commented 1 year ago

Awesome!

If you can add a test for this, happy to merge it.

flamingbear commented 1 year ago

There's some CI permission stuff it looks like I'll have to dig into. Thanks.

rabernat commented 1 year ago

I'm guessing that some of the test failures we are seeing are due to a regression with Zarr.

flamingbear commented 1 year ago

Yes, I saw those errors when I was developing and pinned zarr-python at 2.13.3. But the CI is actually falling over when pulling stuff from gitlab, I think I need to add some credentials.

rabernat commented 1 year ago

136 fixed a couple of problems that had accumulated. If you rebase your branch on the latest master, CI should go green. Thanks for your patience. Have not worked on this repo in a while.

codecov[bot] commented 1 year ago

Codecov Report

Merging #134 (844cc75) into master (a3688f9) will increase coverage by 0.04%. The diff coverage is 100.00%.

:exclamation: Current head 844cc75 differs from pull request most recent head cdad4fe. Consider uploading reports for the commit cdad4fe to get more accurate results

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   96.34%   96.38%   +0.04%     
==========================================
  Files          11       11              
  Lines         547      554       +7     
  Branches      105      106       +1     
==========================================
+ Hits          527      534       +7     
  Misses         13       13              
  Partials        7        7              
Impacted Files Coverage Δ
rechunker/api.py 97.22% <100.00%> (+0.09%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rabernat commented 1 year ago

Perfect, CI is back in good shape. Now we just need a test that covers this new feature and we're good to go! 🚀

Let me know if I can provide any guidance on that.

flamingbear commented 1 year ago

Appreciate it. I will take a look and see if I can make sense of the tests.

flamingbear commented 1 year ago

@rabernat I hope this was you were looking for. Hmm. I thought I saw a bad coverage pop up. I didn't unit test, but added a second group test where it seemed reasonable to me.

rabernat commented 1 year ago

I just released 0.5.1, which has this in it.