-
Notifications
You must be signed in to change notification settings - Fork 97
Make all codecs pickleable #745
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
base: main
Are you sure you want to change the base?
Make all codecs pickleable #745
Conversation
numcodecs/tests/test_zarr3.py
Outdated
print(codec) | ||
print(codec.codec_name) | ||
print(codec.__doc__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for numcodecs.zarr3.Blosc
this returns
Blosc(codec_name='numcodecs.Blosc', codec_config={})
numcodecs.Blosc
See :class:`numcodecs.blosc.Blosc` for more details and parameters.
which all seems correct to me?
numcodecs/zarr3.py
Outdated
@@ -184,30 +195,18 @@ async def _encode_single(self, chunk_ndbuffer: NDBuffer, chunk_spec: ArraySpec) | |||
|
|||
|
|||
def _add_docstring(cls: type[T], ref_class_name: str) -> type[T]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this can be deleted once the changes in this PR are applied to all the other codecs
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #745 +/- ##
==========================================
- Coverage 99.96% 99.96% -0.01%
==========================================
Files 63 63
Lines 2736 2708 -28
==========================================
- Hits 2735 2707 -28
Misses 1 1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomNicholas, I believe my suggested changes should get the tests to pass.
Co-authored-by: Chuck Daniels <[email protected]>
You legend @chuckwondo 🙇♂️ |
pre-commit.ci autofix |
@@ -362,19 +286,39 @@ def evolve_from_array_spec(self, array_spec: ArraySpec) -> AsType: | |||
|
|||
|
|||
# bytes-to-bytes checksum codecs | |||
CRC32 = _add_docstring(_make_checksum_codec("crc32", "CRC32"), "numcodecs.checksum32.CRC32") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These codecs break the pattern of the ones above because they have "numcodecs.checksum32.<name>"
instead of "numcodecs.<lowercasename>.<name>"
. I ignored that, but it doesn't seem to matter whatsoever?? Suspicious.
@d-v-b @jakirkham this is ready for review. It would be nice to release it quickly if possible 😁 |
import numcodecs.bitround | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember adding this - maybe the linter did??
Attempt to fix #744, using the
__init_subclass__
approach suggested by @d-v-b.I think this approach seems promising, but currently tests fail, and I don't know why. I get errors like
TODO: