Skip to content

Fix N5Store dtype wrong behavior #1340

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
Merged
merged 12 commits into from
Feb 20, 2023
Merged

Fix N5Store dtype wrong behavior #1340

merged 12 commits into from
Feb 20, 2023

Conversation

mzouink
Copy link
Contributor

@mzouink mzouink commented Feb 3, 2023 •
edited
Loading

[Description of PR]
Fixing issue: #1339

@d-v-b

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Feb 3, 2023
@joshmoore
Copy link
Member

Launched the workflows. Thanks, @mzouink!

d-v-b
d-v-b reviewed Feb 3, 2023
View reviewed changes
zarr/tests/test_n5.py Outdated
z = zarr.open(n5_store)
z.create_dataset("test", shape=100, dtype="u1")
dtype_n5 = json.loads(n5_store["test/.zarray"])["dtype"]
shutil.rmtree(store)
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using atexit.register here instead, e.g.

zarr-python/zarr/tests/test_storage.py

Line 992 in 4dc6f1f

atexit.register(atexit_rmtree, path)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! i am using it now

@d-v-b
Copy link
Contributor

d-v-b commented Feb 3, 2023

I feel like this case should be caught by existing tests. I will look through the test code later and see why this isn't happening.

@mzouink
Copy link
Contributor Author

mzouink commented Feb 6, 2023 •
edited
Loading

@d-v-b
The test is feeling in

zarr-python/zarr/tests/test_core.py

Lines 1734 to 1741 in 4dc6f1f

def expected(self):
return [
"d174aa384e660eb51c6061fc8d20850c1159141f",
"125f00eea40032f16016b292f6767aa3928c00a7",
"1b52ead0ed889a781ebd4db077a29e35d513c1f3",
"719a88b34e362ff65df30e8f8810c1146ab72bc1",
"6e0abf30daf45de51593c227fb907759ca725551",
]

zarr/tests/test_core.py::TestArrayWithN5Store::test_hexdigest - AssertionError: assert ['4e9cf910000...e27193c995e2'] == ['8811a77d54c...d30c80dfc372']

And it is hardcoded + overwritten for N5. I don't think it is the right way to do it

@joshmoore
Copy link
Member

Re-launched the workflows. Just a heads up, @mzouink, @d-v-b did a good deal of cleaning of test_core.py in https://github.com/zarr-developers/zarr-python/pull/1305/files#diff-acf894bdb70842660ec996213f7ff01514172d0fd4208cbc33a942929e1c7cb2 in case you want to compare there.

@codecov
Copy link

codecov bot commented Feb 10, 2023 •
edited
Loading

Codecov Report

Merging #1340 (595b747) into main (c375030) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1340   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        36           
  Lines        14752     14768   +16     
=========================================
+ Hits         14752     14768   +16
Impacted Files Coverage Δ
zarr/n5.py 100.00% <100.00%> (ø)
zarr/tests/test_core.py 100.00% <100.00%> (ø)
zarr/tests/test_n5.py 100.00% <100.00%> (ø)

joshmoore
joshmoore reviewed Feb 10, 2023
View reviewed changes
zarr/tests/test_n5.py
@@ -35,3 +39,14 @@ def test_partial_chunk_decode(chunk_shape: Tuple[int, ...]):
chunk[subslices] = 1
subchunk = np.ascontiguousarray(chunk[subslices])
assert np.array_equal(codec_wrapped.decode(codec_wrapped.encode(subchunk)), chunk)


def test_dtype_decode():
Copy link
Member

Choose a reason for hiding this comment

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

This test will need the fsspec check:

@pytest.mark.skipif(have_fsspec is False, reason="needs fsspec")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! i didn't know how to do it. i added it.

Copy link
Member

Choose a reason for hiding this comment

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

Cheers!

@joshmoore
Copy link
Member

The test that was failing is now passing. Thanks, @mzouink. @d-v-b, do you want to sign off on this?

@mzouink
Copy link
Contributor Author

mzouink commented Feb 10, 2023

yes @joshmoore. i think it is ready now. thank you for your time.

@d-v-b
Copy link
Contributor

d-v-b commented Feb 13, 2023

looks good to me

@joshmoore
Copy link
Member

Great, thanks all. I'll get this into the next release. @mzouink, please feel free to add your contribution to docs/release.rst if you would like.

@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Feb 14, 2023
@joshmoore
Copy link
Member

🎉

@joshmoore joshmoore merged commit d7d8815 into zarr-developers:main Feb 20, 2023
@sanketverma1704 sanketverma1704 mentioned this pull request Feb 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshmoore joshmoore joshmoore left review comments

@d-v-b d-v-b d-v-b 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.
3 participants
@mzouink @joshmoore @d-v-b