Skip to content

Fix newly introduced bug when opening Zarr arrays#51

Merged
felixcremer merged 5 commits intomasterfrom
fg/zarrkwargs
Mar 30, 2026
Merged

Fix newly introduced bug when opening Zarr arrays#51
felixcremer merged 5 commits intomasterfrom
fg/zarrkwargs

Conversation

@meggart
Copy link
Copy Markdown
Member

@meggart meggart commented Mar 26, 2026

No description provided.

@felixcremer
Copy link
Copy Markdown
Member

We should add a test for that.

@felixcremer
Copy link
Copy Markdown
Member

I added a test for that and switched the test data to using esdc.
Surprisingly the comparison between the to_dataset results are failing, that is why I switched to doing the tests in a for loop on the different loading ways.

@meggart
Copy link
Copy Markdown
Member Author

meggart commented Mar 30, 2026

Any idea where the netcdf-related error is coming from? Is it related to this change?

@felixcremer
Copy link
Copy Markdown
Member

I don't think it is coming from these changes, because we don't touch the Netcdf path at all. I actually wanted to ask you the same.

@meggart
Copy link
Copy Markdown
Member Author

meggart commented Mar 30, 2026

@felixcremer Looks like in the test we are trying to store a Rational in a NetCDF file, this can not work. Did this work in previous versions?

@felixcremer
Copy link
Copy Markdown
Member

Ups. Then I broke the tests by adding to much tests. I think that didn't work in previous versions. Then I broke the tests in #50.

@felixcremer
Copy link
Copy Markdown
Member

I fixed it and the tests pass locally. I had to do a force-push since I merged master locally.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 51.44%. Comparing base (079e030) to head (49e0947).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
ext/ZarrExt.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
+ Coverage   51.03%   51.44%   +0.40%     
==========================================
  Files          12       12              
  Lines         482      484       +2     
==========================================
+ Hits          246      249       +3     
+ Misses        236      235       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@felixcremer
Copy link
Copy Markdown
Member

The failure on windows nightly and stable seem to be an unrelated downloading time out error which comes from Downloads.download.

@felixcremer felixcremer merged commit f309830 into master Mar 30, 2026
9 of 11 checks passed
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.

2 participants