Register BitRound (fix #346)

[joshmoore]

#299 was missing the registration of the class itself. We likely need a test to detect all classes that weren't registered.

TODO:

• Unit tests and/or doctests in docstrings
• tox -e py310 passes locally
• Docstrings and API docs for any new/modified user-facing classes and functions
• Changes documented in docs/release.rst
• tox -e docs passes locally
• GitHub Actions CI passes
• Test coverage to 100% (Coveralls passes)

[pep8speaks]

Hello @joshmoore! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file numcodecs/__init__.py:

Line 103:1: E402 module level import not at top of file

Comment last updated at 2022-08-01 18:53:49 UTC

[joshmoore]

ignoring pep8speaks in favor of keeping the file consistent, though I imagine that it could use some cleanup, e.g. alphabetical ordering of the registrations.

[martindurant]

Should it not now be enough to put it in the entrypoints? Or maybe there's no point doing that because we know numcodecs must be imported.

[joshmoore]

We could certainly replace all of the imports in __init__.py with entrypoints. That won't help with this case though because we'd still want a test that shows that new classes were added to setup.py. The other potential downside is I've been having trouble having entrypoints registered with pip install -e . (Help welcome)

[martindurant]

OK, well let's not for this PR. For record, an entrypoints-only (or importlib.metadata.entry_points()) method would have

• pros: no need for all the import and try/excepts; and no need to import any of the submodules at all except when needed, maybe even leading to a pure python builtins-only numcodec option
• cons: take some time to scan entryponts but then end up making the imports of numcodecs modules anyway. If entrypoints is only the fallback, we can defer that import until we need it.

I've been having trouble having entrypoints registered

Certainly let's fix this!

[jakirkham]

Do we want to include a release note?

[joshmoore]

Just waiting on the RTD for review and then I'll release 0.10.2.

[jakirkham]

Do we know why tests didn't catch this?

[joshmoore]

Ignoring the Windows failure:

Traceback (most recent call last):
      File "C:\Miniconda\lib\site-packages\urllib3\response.py", line 438, in _error_catcher
        yield
      File "C:\Miniconda\lib\site-packages\urllib3\response.py", line 767, in read_chunked
        chunk = self._handle_chunk(amt)
      File "C:\Miniconda\lib\site-packages\urllib3\response.py", line 7[20](https://github.com/zarr-developers/numcodecs/runs/7618040304?check_suite_focus=true#step:4:21), in _handle_chunk
        returned_chunk = self._fp._safe_read(self.chunk_left)
      File "C:\Miniconda\lib\http\client.py", line 626, in _safe_read
        chunk = self.fp.read(min(amt, MAXAMOUNT))
      File "C:\Miniconda\lib\socket.py", line 704, in readinto
        return self._sock.recv_into(b)
      File "C:\Miniconda\lib\ssl.py", line 1[24](https://github.com/zarr-developers/numcodecs/runs/7618040304?check_suite_focus=true#step:4:25)1, in recv_into
        return self.read(nbytes, buffer)
      File "C:\Miniconda\lib\ssl.py", line 1099, in read
        return self._sslobj.read(len, buffer)
    ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host

...

    requests.exceptions.ChunkedEncodingError: ("Connection broken: ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None)", ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None))

knowing that it's unrelated, but there may be an issue in this repo that needs fixing.

[jakirkham]

Wonder if we should test the batteries included codecs as part of the registry tests

[joshmoore]

Do we know why tests didn't catch this?

All current tests specifically do BitRound() rather than loading via get_codec. Catching this with the current setup would require finding every class defined in all modules in this repo and checking that they are findable via get_codec. That test could also be used to say that sample datasets should be stored under https://github.com/zarr-developers/numcodecs/tree/main/fixture (cc: @rabernat)

A simpler fix would likely be to have a "new codec" issue with a dedicated checklist. (cc: @MSanKeys963)

[joshmoore]

Wonder if we should test the batteries included codecs as part of the registry tests

For example, yeah 👍

[jakirkham]

Filed issue ( #348 ) as a placeholder linking to this testing discussion. Feel free to add anything else there

[jakirkham]

Thanks for handling this Josh! 🙏