Introduce a flat option to ensure_contiguous_ndarray to switch off flatten for ZFPY codec #307

halehawk · 2022-02-11T18:34:55Z

closes #303

TODO:

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

pep8speaks · 2022-02-11T18:34:58Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-03-01 20:52:40 UTC

rabernat

This is a great start @halehawk!

Instead of printing and exiting, you want to raise an error, as suggested below.

You will also need to test for this error in test_zfpy.py using with pytest.raises.

numcodecs/zfpy.py

rabernat · 2022-02-18T16:25:11Z

numcodecs/zfpy.py

-            buf = ensure_contiguous_ndarray(buf)
+            # not flatten c-order array and raise exception for f-order array
+            if not isinstance(buf, np.ndarray):
+                raise TypeError("The zfp codec does not support none numpy arrays."


What other type would you expect here? I thought all Zarr array data would come into numcodecs as numpy arrays?

I would like to repeat my answers here, I added test_err_encode_list, which caused buf.flags not raise a correct error. So I added check the instance first. Do you have any different recommendation?

halehawk · 2022-02-18T16:47:17Z

There is a test_err_encode_list, I cannot pass it if I don't check the type of the object.

…

On Fri, Feb 18, 2022 at 9:25 AM Ryan Abernathey ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In numcodecs/zfpy.py <#307 (comment)> : > @@ -54,8 +55,16 @@ def __init__( def encode(self, buf): - # normalise inputs - buf = ensure_contiguous_ndarray(buf) + # not flatten c-order array and raise exception for f-order array + if not isinstance(buf, np.ndarray): + raise TypeError("The zfp codec does not support none numpy arrays." What other type would you expect here? I thought all Zarr array data would come into numcodecs as numpy arrays? — Reply to this email directly, view it on GitHub <#307 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAPEFCWGARTLSEC7QPPEQ3U3ZXHHANCNFSM5OFA7XAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

rabernat

Thanks for your work on this! LGTM!

rabernat · 2022-02-22T20:58:46Z

One final question I have (which doesn't have to hold up this PR) is whether you have tested this branch on a real-world use case and verified that it leads to much better compression, as expected from the discussion in #303.

halehawk · 2022-02-22T21:51:38Z

@rabernat I checked both numcodecs 0.9.1 which has flatten array and my modified numcodecs. The non-flatten array definitely can provide us better compression ratio, for example using precison mode 16 bits, we can get from 2.28 to 4.32 on a 4D array, though the error metric might be not that good, such as: rmse from 0.003979 to 0.05241.

halehawk · 2022-02-22T23:25:45Z

Looks like Peter's group added some meta data about macos-python3.10-zfpy build, but didn't upload the package yet. So I got download problem with this zfpy package.

halehawk · 2022-02-24T16:06:21Z

Looks like zfpy-0.5.5-cp310-macos can be pip install successfully now, this time the failed OSX CI/build (3.10) should be able to pass. How can I restart all PR checks?

halehawk · 2022-02-25T16:11:46Z

Now my PR failed on macos-py310 check again, but the problem was happened during building blosc.cpython-310-darwin.so. @rabernat @jakirkham Do you have any advice on this? Can you ignore this failure and merge my PR now? Thanks!

rabernat · 2022-02-25T16:23:41Z

I do not know what is causing the OSX py 3.10 failure.

In general we do not like to merge things with failing CI, but I would defer to John's judgement on this.

jakirkham · 2022-02-25T21:24:27Z

Regarding CI. Fixing in PR ( #311 )

jakirkham · 2022-02-25T21:45:21Z

.github/workflows/ci-osx.yaml

-          conda create -n env python==${{matrix.python-version}} wheel pip compilers
+          conda create -n env python=${{matrix.python-version}} wheel pip compilers 'clang>=12.0.1'


Let's leave this for that PR to sort out 🙂

You want to me revert the modification?

rabernat · 2022-02-28T17:39:48Z

LGTM, but i would appreciate another maintainer approval.

jakirkham

Generally LGTM. Had a small suggestion on parameter naming. Once done would be happy to merge 🙂

Thanks for all the work here @halehawk! 😄

numcodecs/compat.py

halehawk · 2022-03-01T20:28:51Z

Done. Thanks for the feedback.

…

On Tue, Mar 1, 2022 at 12:55 PM jakirkham ***@***.***> wrote: ***@***.**** commented on this pull request. Generally LGTM. Had a small suggestion on parameter naming. Once done would be happy to merge 🙂 Thanks for all the work here @halehawk <https://github.com/halehawk>! 😄 ------------------------------ In numcodecs/compat.py <#307 (comment)> : > @@ -50,7 +50,7 @@ def ensure_ndarray(buf): return arr -def ensure_contiguous_ndarray(buf, max_buffer_size=None): +def ensure_contiguous_ndarray(buf, max_buffer_size=None, flat=True): Preference for using a verb to describe the action taken or not ⬇️ Suggested change -def ensure_contiguous_ndarray(buf, max_buffer_size=None, flat=True): +def ensure_contiguous_ndarray(buf, max_buffer_size=None, flatten=True): ------------------------------ In numcodecs/compat.py <#307 (comment)> : > + # check if flat flag is on or not + if flat: ⬇️ Suggested change - # check if flat flag is on or not - if flat: + # check if flatten flag is on or not + if flatten: — Reply to this email directly, view it on GitHub <#307 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAPEFHSU5TNJYNYFMGAMP3U5ZYZNANCNFSM5OFA7XAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

jakirkham

Think this should fix CI

numcodecs/zfpy.py

jakirkham · 2022-03-02T05:57:40Z

Thanks @halehawk for the PR and everyone for the reviews! 😄

halehawk added 3 commits February 10, 2022 14:34

Optional flatten the encode array

ec1058f

Remove skipping Python 3.9 and later for Zfp

80cf195

Updated docs/release.rst about ZFPY

7038e96

halehawk added 4 commits February 11, 2022 11:37

Merge branch 'master' into master

9d5d7bd

Removed the trailing whitespace

39a58ef

Merge branch 'master' of https://github.com/pangeo-data/numcodecs

54e2529

Add pip install zfpy to start test_zfpy

7e27429

halehawk mentioned this pull request Feb 11, 2022

Data array is flattened in numcodecs which reduced the compression ratio that ZFP can provide on multi-dimension arrays #303

Closed

rabernat requested changes Feb 14, 2022

View reviewed changes

numcodecs/zfpy.py Outdated Show resolved Hide resolved

halehawk added 2 commits February 15, 2022 11:09

Add raise valueerror

0f68801

Shorten the raise error lines

9bdc0c3

rabernat reviewed Feb 18, 2022

View reviewed changes

rabernat approved these changes Feb 22, 2022

View reviewed changes

Merge branch 'master' into master

d5d32a0

Modified ci-osx clang to 12.0.1

18184d7

jakirkham reviewed Feb 25, 2022

View reviewed changes

halehawk added 2 commits February 25, 2022 16:17

Revert the change in ci-osx.yaml

1824166

Merge branch 'master' into master

cbd2d0c

halehawk added 2 commits March 1, 2022 09:47

Update release.rst

91640cd

Merge branch 'master' of https://github.com/pangeo-data/numcodecs

11ba699

jakirkham reviewed Mar 1, 2022

View reviewed changes

numcodecs/compat.py Outdated Show resolved Hide resolved

numcodecs/compat.py Outdated Show resolved Hide resolved

Change flat to flatten

2a79fd6

jakirkham approved these changes Mar 1, 2022

View reviewed changes

Missed one flat, changed to flatten

cb978d8

jakirkham reviewed Mar 1, 2022

View reviewed changes

numcodecs/zfpy.py Outdated Show resolved Hide resolved

numcodecs/zfpy.py Outdated Show resolved Hide resolved

Change flat to flatten in zfpy.py

894324d

jakirkham merged commit f4196a4 into zarr-developers:master Mar 2, 2022

		conda create -n env python==${{matrix.python-version}} wheel pip compilers
		conda create -n env python=${{matrix.python-version}} wheel pip compilers 'clang>=12.0.1'

Introduce a flat option to ensure_contiguous_ndarray to switch off flatten for ZFPY codec #307

Introduce a flat option to ensure_contiguous_ndarray to switch off flatten for ZFPY codec #307

Uh oh!

Conversation

halehawk commented Feb 11, 2022 • edited by rabernat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2022-03-01 20:52:40 UTC

Uh oh!

rabernat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rabernat Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

halehawk Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

halehawk commented Feb 18, 2022 via email

Uh oh!

rabernat left a comment

Choose a reason for hiding this comment

Uh oh!

rabernat commented Feb 22, 2022

Uh oh!

halehawk commented Feb 22, 2022

Uh oh!

halehawk commented Feb 22, 2022

Uh oh!

halehawk commented Feb 24, 2022

Uh oh!

halehawk commented Feb 25, 2022

Uh oh!

rabernat commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakirkham commented Feb 25, 2022

Uh oh!

jakirkham Feb 25, 2022

Choose a reason for hiding this comment

Uh oh!

halehawk Feb 25, 2022

Choose a reason for hiding this comment

Uh oh!

jakirkham Feb 25, 2022

Choose a reason for hiding this comment

Uh oh!

rabernat commented Feb 28, 2022

Uh oh!

jakirkham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

halehawk commented Mar 1, 2022 via email

Uh oh!

jakirkham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jakirkham commented Mar 2, 2022

Uh oh!

Uh oh!

halehawk commented Feb 11, 2022 •

edited by rabernat

Loading

pep8speaks commented Feb 11, 2022 •

edited

Loading

rabernat commented Feb 25, 2022 •

edited

Loading