-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix: ZarrAvgMerger ValueError with zarr_format 3 #8477
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: ZarrAvgMerger ValueError with zarr_format 3 #8477
Conversation
My understanding was that this argument was left in place to be compatible with zarr 2 as well as zarr 3. There was discussion on this here. A related issue raised in #8476 in our tests occurs when using zarr 3 with our tests, it would be nice to integrate a fix in this test by adding logic to remove this argument here and replace it with a use of the |
Hi @kolasaniv1996, I wanted to check in to see if you’re planning to complete this PR. It seems it might be a blocker for our v1.5 release. Let me know if there's anything we can do to assist. Thanks. |
@KumoLiu I'm working on it |
c7177c3
to
b09f93a
Compare
Signed-off-by: kolasaniv1996 <[email protected]>
02bc380
to
708aec8
Compare
- Added deprecated_arg decorator for compressor, value_compressor, and count_compressor (since 1.5.0, to be removed in 1.7.0) - Added codecs, value_codecs, and count_codecs support for zarr format 3 - Updated tests to handle both zarr v2 and zarr v3 compatibility - Fixed issue where tests would fail with zarr v3 due to compressor usage Signed-off-by: kolasaniv1996 <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: kolasaniv1996 <[email protected]>
CI error log:
|
…ons and updating test assertions Signed-off-by: kolasaniv1996 <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: kolasaniv1996 <[email protected]>
…ects Signed-off-by: kolasaniv1996 <[email protected]>
93d2146
to
389b300
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Signed-off-by: vivek kolasani <[email protected]>
Signed-off-by: vivek kolasani <[email protected]>
Signed-off-by: vivek kolasani <[email protected]>
Signed-off-by: YunLiu <[email protected]>
Hi @kolasaniv1996 and @ericspod, I try to fix the ci issue in this commit: cac8923 BTW @kolasaniv1996, could you please follow this guide to fix the DCO issue: https://github.com/Project-MONAI/MONAI/pull/8477/checks?check_run_id=44029533563? Thanks! |
Signed-off-by: YunLiu <[email protected]>
Signed-off-by: YunLiu <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: YunLiu <[email protected]>
for more information, see https://pre-commit.ci
/build |
This PR fixes the issue with ZarrAvgMerger when using zarr v3, where the compressor argument is not supported.
Changes:
This addresses the issue reported in #8476 where using compressor with zarr v3 causes ValueError.
Signed-off-by: kolasaniv1996 [email protected]