Skip to content

Ensure compressor=None results in no compression for V2 #2709

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 7 commits into from
Jan 14, 2025

Conversation

martindurant
Copy link
Member

Fixes #2708

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)

cc @d-v-b

martindurant and others added 2 commits January 14, 2025 11:44
Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com>
@d-v-b d-v-b requested a review from normanrz January 14, 2025 17:03
Copy link
Member

@normanrz normanrz left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

"""
g = zarr.open(store, mode="w", zarr_format=2)
arr = g.create_array("one", dtype="i8", shape=(1,), chunks=(1,), compressor=None)
assert arr._async_array.compressor is None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert arr._async_array.compressor is None
assert arr.compressor is None

Does this work, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but that's deprecated, apparently

Copy link
Member

Choose a reason for hiding this comment

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

So should be asyncarray.compressor. You can also check array.compressors

@dstansby dstansby added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 14, 2025
@martindurant
Copy link
Member Author

@dstansby , added release note

@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Jan 14, 2025
@dstansby dstansby merged commit 45146ca into zarr-developers:main Jan 14, 2025
30 checks passed
@dstansby
Copy link
Contributor

Thanks!

@d-v-b
Copy link
Contributor

d-v-b commented Feb 12, 2025

@martindurant I only just noticed that this PR added a new keyword argument (compressor) to create_array, which we definitely do not want (the idea is that v2 and v3 creation can both be handled by compressors). How painful would it be to migrate away from compressor, if we removed it?

@d-v-b
Copy link
Contributor

d-v-b commented Feb 12, 2025

i should have noticed this in review, but the compressor/ compressors distinction was too subtle for me

@martindurant
Copy link
Member Author

Kerchunk can cope, but this was following the v2 pattern rather than new. The handling of the argument is specifically dependent on the format version requested.

Personally, I would have preferred "filters" or something more specific like "codecs". The filters/compressor split before was roughly the same at the array->array/bytes->bytes split we have now.

@martindurant martindurant deleted the default_v2_comprssor branch February 12, 2025 15:24
@d-v-b
Copy link
Contributor

d-v-b commented Feb 12, 2025

create_array takes filters, and it maps on to array-array codecs in the v3 case

@d-v-b
Copy link
Contributor

d-v-b commented Feb 12, 2025

i opened #2818 to propose removing the compressor kwarg

@martindurant
Copy link
Member Author

Right; but before, filters was the whole pipeline. In fact, it was stored like that, and compressor= just became the final element.

@d-v-b
Copy link
Contributor

d-v-b commented Feb 12, 2025

it should still be possible to pass filters=<everything>, compressors=None, and get that outcome.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In V2, creating an array with compressor=None assumes Zstd
4 participants