Use `ensure_ndarray` in a few more places by jakirkham · Pull Request #506 · zarr-developers/zarr-python

jakirkham · 2019-11-11T19:48:53Z

As we can coerce any object that is array-like to an ndarray, there should be no need for these explicit ndarray checks. So eliminate them by using ensure_ndarray to get an ndarray object instead.

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
Docs build locally (e.g., run tox -e docs)
AppVeyor and Travis CI passes
Test coverage is 100% (Coveralls passes)

Instead of checking to see if `chunk` is an `ndarray`, use `ensure_ndarray` to get an `ndarray` viewing the underlying data in `chunk`. This way we know attributes like `dtype` are available and can be checked easily. Also makes this a bit more friendly with other array-like types.

Since we are interested in getting an `ndarray` representing the buffer within `out` for writing into, go ahead and use `ensure_ndarray` to coerce the underlying buffer into an `ndarray`. This way we can avoid a needless check and just write into any array-like value for `out` that is provided.

Appears that `out` can also be a Zarr `Array` or any other array-like that does not expose a buffer, but does allow writing into. In these cases `ensure_ndarray` will fail as there is not an underlying buffer that can be used with an `ndarray`. To also handle this case, catch the `TypeError` that `ensure_ndarray` will raise in this case and use that to indicate whether `out` is now an `ndarray` or not. This allows us to continue to write into arbitrary buffers, but also correctly handle objects that do not expose buffers.

jakirkham · 2019-11-11T21:57:38Z

@jrbourbeau, if you are around, this could use a review 🙂

jakirkham · 2019-11-11T23:08:22Z


+ out_is_ndarray = True
+ try:
+ out = ensure_ndarray(out)


Note that this is already checked by test_get_selection_out. Without this try/except, we get the following TypeError because out is a Zarr Array, which cannot be coerced to an ndarray. This is ok and intentional. So we just carry on without coercing that case and note that we do not have an ndarray when checking later.
Details
_________________________test_get_selection_out____________________________deftest_get_selection_out(): # basic selectionsa=np.arange(1050) z=zarr.create(shape=1050, chunks=100, dtype=a.dtype) z[:] =aselections= [ slice(50, 150), slice(0, 1050), slice(1, 2), ] forselectioninselections: expect=a[selection] out=zarr.create(shape=expect.shape, chunks=10, dtype=expect.dtype, fill_value=0) >z.get_basic_selection(selection, out=out) zarr/tests/test_indexing.py:1036: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:698: inget_basic_selectionfields=fields) zarr/core.py:740: in_get_basic_selection_ndreturnself._get_selection(indexer=indexer, out=out, fields=fields) zarr/core.py:1028: in_get_selectiondrop_axes=indexer.drop_axes, fields=fields) zarr/core.py:1573: in_chunk_getitemout=ensure_ndarray(out) __ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ buf=<zarr.core.Array (100,) int64>defensure_ndarray(buf): """Convenience function to coerce `buf` to a numpy array, if it is not already a numpy array. Parameters ---------- buf : array-like or bytes-like A numpy array or any object exporting a buffer interface. Returns ------- arr : ndarray A numpy array, sharing memory with `buf`. Notes ----- This function will not create a copy under any circumstances, it is guaranteed to return a view on memory exported by `buf`. """ifisinstance(buf, np.ndarray): # already a numpy arrayarr=bufelifisinstance(buf, array.array) andbuf.typecodein'cu': # Guard condition, do not support array.array with unicode type, this is# problematic because numpy does not support it on all platforms. Also do not# support char as it was removed in Python 3.raiseTypeError('array.array with char or unicode type is not supported') else: # N.B., first take a memoryview to make sure that we subsequently create a# numpy array from a memory buffer with no copyifPY2: # pragma: py3 no covertry: mem=memoryview(buf) exceptTypeError: # on PY2 also check if object exports old-style buffer interfacemem=np.getbuffer(buf) else: # pragma: py2 no cover>mem=memoryview(buf) ETypeError: memoryview: abytes-likeobjectisrequired, not'Array' .tox/py36/lib/python3.6/site-packages/numcodecs/compat.py:74: TypeError
ref: https://travis-ci.org/zarr-developers/zarr-python/jobs/610533967#L2316

jakirkham · 2019-11-12T02:00:21Z

Planning on merging EOD tomorrow if no comments.

jrbourbeau

Thanks for the PR @jakirkham! Generally these changes seem fine by me. I have one question about when ensure_ndarray raises an error that I've left below.

jrbourbeau · 2019-11-12T02:18:42Z


 # check object encoding
- if isinstance(chunk, np.ndarray) and chunk.dtype == object:
+ if ensure_ndarray(chunk).dtype == object:


Is it possible that some non-ndarrays would previously skip over this if block but now fail if ensure_ndarray raises an error? For example:
In [28]: fromnumcodecs.compatimportensure_ndarrayIn [29]: importarrayIn [30]: chunk=array.array('u', 'hello \u2641') In [31]: ifisinstance(chunk, np.ndarray) andchunk.dtype==object: ...: pass ...: In [32]: ifensure_ndarray(chunk).dtype==object: ...: pass ...: ---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast) <ipython-input-32-e1e15cbf81c2>in<module>---->1ifensure_ndarray(chunk).dtype==object: 2pass3~/miniconda/envs/zarr-python-dev/lib/python3.7/site-packages/numcodecs/compat.pyinensure_ndarray(buf) 63# problematic because numpy does not support it on all platforms. Also do not64# support char as it was removed in Python 3.--->65raiseTypeError('array.array with char or unicode type is not supported') 6667else: TypeError: array.arraywithcharorunicodetypeisnotsupported
To be clear, I'm not sure how likely this is (or if it's even possible) to come up in practice. Do you have a sense for this?

That exception would be expected as we have decided not to work with Python builtin arrays that use character or unicode types. There is some more detailed discussion in this thread.

Basically the only exception that we need to worry about is the one coming from memoryview, which will be a TypeError if the object doesn't support the buffer protocol. After that point we have a memoryview. So we know the rest of the code will work (unless NumPy gets a bug ;).
Edit: Sorry was looking at the other change for a second.

For context this function is trying to turn some data into bytes that can be serialized (say to a file on disk).
The filters before this step should be returning something that either is an ndarray or could be coerced to one. This is required by the compressors that follow and the storage layer afterwards. So if ensure_ndarray fails here, then we have invalid data and raising an exception to the user would be appropriate.

…y_more

jrbourbeau

Great, thank you for providing all the additional context surrounding this section. I pushed a commit with a changelog entry. Feel free to merge on green. Thanks @jakirkham!

jakirkham · 2019-11-12T18:56:28Z

Thanks @jrbourbeau for the review and fix! 😄

jakirkham · 2019-11-11T19:48:53Z

As we can coerce any object that is array-like to an ndarray, there should be no need for these explicit ndarray checks. So eliminate them by using ensure_ndarray to get an ndarray object instead.

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
Docs build locally (e.g., run tox -e docs)
AppVeyor and Travis CI passes
Test coverage is 100% (Coveralls passes)

Instead of checking to see if `chunk` is an `ndarray`, use `ensure_ndarray` to get an `ndarray` viewing the underlying data in `chunk`. This way we know attributes like `dtype` are available and can be checked easily. Also makes this a bit more friendly with other array-like types.

Since we are interested in getting an `ndarray` representing the buffer within `out` for writing into, go ahead and use `ensure_ndarray` to coerce the underlying buffer into an `ndarray`. This way we can avoid a needless check and just write into any array-like value for `out` that is provided.

Appears that `out` can also be a Zarr `Array` or any other array-like that does not expose a buffer, but does allow writing into. In these cases `ensure_ndarray` will fail as there is not an underlying buffer that can be used with an `ndarray`. To also handle this case, catch the `TypeError` that `ensure_ndarray` will raise in this case and use that to indicate whether `out` is now an `ndarray` or not. This allows us to continue to write into arbitrary buffers, but also correctly handle objects that do not expose buffers.

jakirkham · 2019-11-11T21:57:38Z

@jrbourbeau, if you are around, this could use a review 🙂

jakirkham · 2019-11-11T23:08:22Z


+ out_is_ndarray = True
+ try:
+ out = ensure_ndarray(out)


Note that this is already checked by test_get_selection_out. Without this try/except, we get the following TypeError because out is a Zarr Array, which cannot be coerced to an ndarray. This is ok and intentional. So we just carry on without coercing that case and note that we do not have an ndarray when checking later.
Details
_________________________test_get_selection_out____________________________deftest_get_selection_out(): # basic selectionsa=np.arange(1050) z=zarr.create(shape=1050, chunks=100, dtype=a.dtype) z[:] =aselections= [ slice(50, 150), slice(0, 1050), slice(1, 2), ] forselectioninselections: expect=a[selection] out=zarr.create(shape=expect.shape, chunks=10, dtype=expect.dtype, fill_value=0) >z.get_basic_selection(selection, out=out) zarr/tests/test_indexing.py:1036: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:698: inget_basic_selectionfields=fields) zarr/core.py:740: in_get_basic_selection_ndreturnself._get_selection(indexer=indexer, out=out, fields=fields) zarr/core.py:1028: in_get_selectiondrop_axes=indexer.drop_axes, fields=fields) zarr/core.py:1573: in_chunk_getitemout=ensure_ndarray(out) __ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ buf=<zarr.core.Array (100,) int64>defensure_ndarray(buf): """Convenience function to coerce `buf` to a numpy array, if it is not already a numpy array. Parameters ---------- buf : array-like or bytes-like A numpy array or any object exporting a buffer interface. Returns ------- arr : ndarray A numpy array, sharing memory with `buf`. Notes ----- This function will not create a copy under any circumstances, it is guaranteed to return a view on memory exported by `buf`. """ifisinstance(buf, np.ndarray): # already a numpy arrayarr=bufelifisinstance(buf, array.array) andbuf.typecodein'cu': # Guard condition, do not support array.array with unicode type, this is# problematic because numpy does not support it on all platforms. Also do not# support char as it was removed in Python 3.raiseTypeError('array.array with char or unicode type is not supported') else: # N.B., first take a memoryview to make sure that we subsequently create a# numpy array from a memory buffer with no copyifPY2: # pragma: py3 no covertry: mem=memoryview(buf) exceptTypeError: # on PY2 also check if object exports old-style buffer interfacemem=np.getbuffer(buf) else: # pragma: py2 no cover>mem=memoryview(buf) ETypeError: memoryview: abytes-likeobjectisrequired, not'Array' .tox/py36/lib/python3.6/site-packages/numcodecs/compat.py:74: TypeError
ref: https://travis-ci.org/zarr-developers/zarr-python/jobs/610533967#L2316

jakirkham · 2019-11-12T02:00:21Z

Planning on merging EOD tomorrow if no comments.

jrbourbeau

Thanks for the PR @jakirkham! Generally these changes seem fine by me. I have one question about when ensure_ndarray raises an error that I've left below.

jrbourbeau · 2019-11-12T02:18:42Z


 # check object encoding
- if isinstance(chunk, np.ndarray) and chunk.dtype == object:
+ if ensure_ndarray(chunk).dtype == object:


Is it possible that some non-ndarrays would previously skip over this if block but now fail if ensure_ndarray raises an error? For example:
In [28]: fromnumcodecs.compatimportensure_ndarrayIn [29]: importarrayIn [30]: chunk=array.array('u', 'hello \u2641') In [31]: ifisinstance(chunk, np.ndarray) andchunk.dtype==object: ...: pass ...: In [32]: ifensure_ndarray(chunk).dtype==object: ...: pass ...: ---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast) <ipython-input-32-e1e15cbf81c2>in<module>---->1ifensure_ndarray(chunk).dtype==object: 2pass3~/miniconda/envs/zarr-python-dev/lib/python3.7/site-packages/numcodecs/compat.pyinensure_ndarray(buf) 63# problematic because numpy does not support it on all platforms. Also do not64# support char as it was removed in Python 3.--->65raiseTypeError('array.array with char or unicode type is not supported') 6667else: TypeError: array.arraywithcharorunicodetypeisnotsupported
To be clear, I'm not sure how likely this is (or if it's even possible) to come up in practice. Do you have a sense for this?

That exception would be expected as we have decided not to work with Python builtin arrays that use character or unicode types. There is some more detailed discussion in this thread.

Basically the only exception that we need to worry about is the one coming from memoryview, which will be a TypeError if the object doesn't support the buffer protocol. After that point we have a memoryview. So we know the rest of the code will work (unless NumPy gets a bug ;).
Edit: Sorry was looking at the other change for a second.

For context this function is trying to turn some data into bytes that can be serialized (say to a file on disk).
The filters before this step should be returning something that either is an ndarray or could be coerced to one. This is required by the compressors that follow and the storage layer afterwards. So if ensure_ndarray fails here, then we have invalid data and raising an exception to the user would be appropriate.

…y_more

jrbourbeau

Great, thank you for providing all the additional context surrounding this section. I pushed a commit with a changelog entry. Feel free to merge on green. Thanks @jakirkham!

jakirkham · 2019-11-12T18:56:28Z

Thanks @jrbourbeau for the review and fix! 😄

jakirkham · 2019-11-11T19:48:53Z

As we can coerce any object that is array-like to an ndarray, there should be no need for these explicit ndarray checks. So eliminate them by using ensure_ndarray to get an ndarray object instead.

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
Docs build locally (e.g., run tox -e docs)
AppVeyor and Travis CI passes
Test coverage is 100% (Coveralls passes)

Instead of checking to see if `chunk` is an `ndarray`, use `ensure_ndarray` to get an `ndarray` viewing the underlying data in `chunk`. This way we know attributes like `dtype` are available and can be checked easily. Also makes this a bit more friendly with other array-like types.

Since we are interested in getting an `ndarray` representing the buffer within `out` for writing into, go ahead and use `ensure_ndarray` to coerce the underlying buffer into an `ndarray`. This way we can avoid a needless check and just write into any array-like value for `out` that is provided.

Appears that `out` can also be a Zarr `Array` or any other array-like that does not expose a buffer, but does allow writing into. In these cases `ensure_ndarray` will fail as there is not an underlying buffer that can be used with an `ndarray`. To also handle this case, catch the `TypeError` that `ensure_ndarray` will raise in this case and use that to indicate whether `out` is now an `ndarray` or not. This allows us to continue to write into arbitrary buffers, but also correctly handle objects that do not expose buffers.

jakirkham · 2019-11-11T21:57:38Z

@jrbourbeau, if you are around, this could use a review 🙂

jakirkham · 2019-11-11T23:08:22Z


+ out_is_ndarray = True
+ try:
+ out = ensure_ndarray(out)


Note that this is already checked by test_get_selection_out. Without this try/except, we get the following TypeError because out is a Zarr Array, which cannot be coerced to an ndarray. This is ok and intentional. So we just carry on without coercing that case and note that we do not have an ndarray when checking later.
Details
_________________________test_get_selection_out____________________________deftest_get_selection_out(): # basic selectionsa=np.arange(1050) z=zarr.create(shape=1050, chunks=100, dtype=a.dtype) z[:] =aselections= [ slice(50, 150), slice(0, 1050), slice(1, 2), ] forselectioninselections: expect=a[selection] out=zarr.create(shape=expect.shape, chunks=10, dtype=expect.dtype, fill_value=0) >z.get_basic_selection(selection, out=out) zarr/tests/test_indexing.py:1036: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:698: inget_basic_selectionfields=fields) zarr/core.py:740: in_get_basic_selection_ndreturnself._get_selection(indexer=indexer, out=out, fields=fields) zarr/core.py:1028: in_get_selectiondrop_axes=indexer.drop_axes, fields=fields) zarr/core.py:1573: in_chunk_getitemout=ensure_ndarray(out) __ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ buf=<zarr.core.Array (100,) int64>defensure_ndarray(buf): """Convenience function to coerce `buf` to a numpy array, if it is not already a numpy array. Parameters ---------- buf : array-like or bytes-like A numpy array or any object exporting a buffer interface. Returns ------- arr : ndarray A numpy array, sharing memory with `buf`. Notes ----- This function will not create a copy under any circumstances, it is guaranteed to return a view on memory exported by `buf`. """ifisinstance(buf, np.ndarray): # already a numpy arrayarr=bufelifisinstance(buf, array.array) andbuf.typecodein'cu': # Guard condition, do not support array.array with unicode type, this is# problematic because numpy does not support it on all platforms. Also do not# support char as it was removed in Python 3.raiseTypeError('array.array with char or unicode type is not supported') else: # N.B., first take a memoryview to make sure that we subsequently create a# numpy array from a memory buffer with no copyifPY2: # pragma: py3 no covertry: mem=memoryview(buf) exceptTypeError: # on PY2 also check if object exports old-style buffer interfacemem=np.getbuffer(buf) else: # pragma: py2 no cover>mem=memoryview(buf) ETypeError: memoryview: abytes-likeobjectisrequired, not'Array' .tox/py36/lib/python3.6/site-packages/numcodecs/compat.py:74: TypeError
ref: https://travis-ci.org/zarr-developers/zarr-python/jobs/610533967#L2316

jakirkham · 2019-11-12T02:00:21Z

Planning on merging EOD tomorrow if no comments.

jrbourbeau

Thanks for the PR @jakirkham! Generally these changes seem fine by me. I have one question about when ensure_ndarray raises an error that I've left below.

jrbourbeau · 2019-11-12T02:18:42Z


 # check object encoding
- if isinstance(chunk, np.ndarray) and chunk.dtype == object:
+ if ensure_ndarray(chunk).dtype == object:


Is it possible that some non-ndarrays would previously skip over this if block but now fail if ensure_ndarray raises an error? For example:
In [28]: fromnumcodecs.compatimportensure_ndarrayIn [29]: importarrayIn [30]: chunk=array.array('u', 'hello \u2641') In [31]: ifisinstance(chunk, np.ndarray) andchunk.dtype==object: ...: pass ...: In [32]: ifensure_ndarray(chunk).dtype==object: ...: pass ...: ---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast) <ipython-input-32-e1e15cbf81c2>in<module>---->1ifensure_ndarray(chunk).dtype==object: 2pass3~/miniconda/envs/zarr-python-dev/lib/python3.7/site-packages/numcodecs/compat.pyinensure_ndarray(buf) 63# problematic because numpy does not support it on all platforms. Also do not64# support char as it was removed in Python 3.--->65raiseTypeError('array.array with char or unicode type is not supported') 6667else: TypeError: array.arraywithcharorunicodetypeisnotsupported
To be clear, I'm not sure how likely this is (or if it's even possible) to come up in practice. Do you have a sense for this?

That exception would be expected as we have decided not to work with Python builtin arrays that use character or unicode types. There is some more detailed discussion in this thread.

Basically the only exception that we need to worry about is the one coming from memoryview, which will be a TypeError if the object doesn't support the buffer protocol. After that point we have a memoryview. So we know the rest of the code will work (unless NumPy gets a bug ;).
Edit: Sorry was looking at the other change for a second.

For context this function is trying to turn some data into bytes that can be serialized (say to a file on disk).
The filters before this step should be returning something that either is an ndarray or could be coerced to one. This is required by the compressors that follow and the storage layer afterwards. So if ensure_ndarray fails here, then we have invalid data and raising an exception to the user would be appropriate.

…y_more

jrbourbeau

Great, thank you for providing all the additional context surrounding this section. I pushed a commit with a changelog entry. Feel free to merge on green. Thanks @jakirkham!

jakirkham · 2019-11-12T18:56:28Z

Thanks @jrbourbeau for the review and fix! 😄

jakirkham · 2019-11-11T19:48:53Z

As we can coerce any object that is array-like to an ndarray, there should be no need for these explicit ndarray checks. So eliminate them by using ensure_ndarray to get an ndarray object instead.

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
Docs build locally (e.g., run tox -e docs)
AppVeyor and Travis CI passes
Test coverage is 100% (Coveralls passes)

Instead of checking to see if `chunk` is an `ndarray`, use `ensure_ndarray` to get an `ndarray` viewing the underlying data in `chunk`. This way we know attributes like `dtype` are available and can be checked easily. Also makes this a bit more friendly with other array-like types.

Since we are interested in getting an `ndarray` representing the buffer within `out` for writing into, go ahead and use `ensure_ndarray` to coerce the underlying buffer into an `ndarray`. This way we can avoid a needless check and just write into any array-like value for `out` that is provided.

Appears that `out` can also be a Zarr `Array` or any other array-like that does not expose a buffer, but does allow writing into. In these cases `ensure_ndarray` will fail as there is not an underlying buffer that can be used with an `ndarray`. To also handle this case, catch the `TypeError` that `ensure_ndarray` will raise in this case and use that to indicate whether `out` is now an `ndarray` or not. This allows us to continue to write into arbitrary buffers, but also correctly handle objects that do not expose buffers.

jakirkham · 2019-11-11T21:57:38Z

@jrbourbeau, if you are around, this could use a review 🙂

jakirkham · 2019-11-11T23:08:22Z


+ out_is_ndarray = True
+ try:
+ out = ensure_ndarray(out)


Note that this is already checked by test_get_selection_out. Without this try/except, we get the following TypeError because out is a Zarr Array, which cannot be coerced to an ndarray. This is ok and intentional. So we just carry on without coercing that case and note that we do not have an ndarray when checking later.
Details
_________________________test_get_selection_out____________________________deftest_get_selection_out(): # basic selectionsa=np.arange(1050) z=zarr.create(shape=1050, chunks=100, dtype=a.dtype) z[:] =aselections= [ slice(50, 150), slice(0, 1050), slice(1, 2), ] forselectioninselections: expect=a[selection] out=zarr.create(shape=expect.shape, chunks=10, dtype=expect.dtype, fill_value=0) >z.get_basic_selection(selection, out=out) zarr/tests/test_indexing.py:1036: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:698: inget_basic_selectionfields=fields) zarr/core.py:740: in_get_basic_selection_ndreturnself._get_selection(indexer=indexer, out=out, fields=fields) zarr/core.py:1028: in_get_selectiondrop_axes=indexer.drop_axes, fields=fields) zarr/core.py:1573: in_chunk_getitemout=ensure_ndarray(out) __ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ buf=<zarr.core.Array (100,) int64>defensure_ndarray(buf): """Convenience function to coerce `buf` to a numpy array, if it is not already a numpy array. Parameters ---------- buf : array-like or bytes-like A numpy array or any object exporting a buffer interface. Returns ------- arr : ndarray A numpy array, sharing memory with `buf`. Notes ----- This function will not create a copy under any circumstances, it is guaranteed to return a view on memory exported by `buf`. """ifisinstance(buf, np.ndarray): # already a numpy arrayarr=bufelifisinstance(buf, array.array) andbuf.typecodein'cu': # Guard condition, do not support array.array with unicode type, this is# problematic because numpy does not support it on all platforms. Also do not# support char as it was removed in Python 3.raiseTypeError('array.array with char or unicode type is not supported') else: # N.B., first take a memoryview to make sure that we subsequently create a# numpy array from a memory buffer with no copyifPY2: # pragma: py3 no covertry: mem=memoryview(buf) exceptTypeError: # on PY2 also check if object exports old-style buffer interfacemem=np.getbuffer(buf) else: # pragma: py2 no cover>mem=memoryview(buf) ETypeError: memoryview: abytes-likeobjectisrequired, not'Array' .tox/py36/lib/python3.6/site-packages/numcodecs/compat.py:74: TypeError
ref: https://travis-ci.org/zarr-developers/zarr-python/jobs/610533967#L2316

jakirkham · 2019-11-12T02:00:21Z

Planning on merging EOD tomorrow if no comments.

jrbourbeau

Thanks for the PR @jakirkham! Generally these changes seem fine by me. I have one question about when ensure_ndarray raises an error that I've left below.

jrbourbeau · 2019-11-12T02:18:42Z


 # check object encoding
- if isinstance(chunk, np.ndarray) and chunk.dtype == object:
+ if ensure_ndarray(chunk).dtype == object:


Is it possible that some non-ndarrays would previously skip over this if block but now fail if ensure_ndarray raises an error? For example:
In [28]: fromnumcodecs.compatimportensure_ndarrayIn [29]: importarrayIn [30]: chunk=array.array('u', 'hello \u2641') In [31]: ifisinstance(chunk, np.ndarray) andchunk.dtype==object: ...: pass ...: In [32]: ifensure_ndarray(chunk).dtype==object: ...: pass ...: ---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast) <ipython-input-32-e1e15cbf81c2>in<module>---->1ifensure_ndarray(chunk).dtype==object: 2pass3~/miniconda/envs/zarr-python-dev/lib/python3.7/site-packages/numcodecs/compat.pyinensure_ndarray(buf) 63# problematic because numpy does not support it on all platforms. Also do not64# support char as it was removed in Python 3.--->65raiseTypeError('array.array with char or unicode type is not supported') 6667else: TypeError: array.arraywithcharorunicodetypeisnotsupported
To be clear, I'm not sure how likely this is (or if it's even possible) to come up in practice. Do you have a sense for this?

That exception would be expected as we have decided not to work with Python builtin arrays that use character or unicode types. There is some more detailed discussion in this thread.

Basically the only exception that we need to worry about is the one coming from memoryview, which will be a TypeError if the object doesn't support the buffer protocol. After that point we have a memoryview. So we know the rest of the code will work (unless NumPy gets a bug ;).
Edit: Sorry was looking at the other change for a second.

For context this function is trying to turn some data into bytes that can be serialized (say to a file on disk).
The filters before this step should be returning something that either is an ndarray or could be coerced to one. This is required by the compressors that follow and the storage layer afterwards. So if ensure_ndarray fails here, then we have invalid data and raising an exception to the user would be appropriate.

…y_more

jrbourbeau

Great, thank you for providing all the additional context surrounding this section. I pushed a commit with a changelog entry. Feel free to merge on green. Thanks @jakirkham!

jakirkham · 2019-11-12T18:56:28Z

Thanks @jrbourbeau for the review and fix! 😄

jakirkham · 2019-11-11T19:48:53Z

As we can coerce any object that is array-like to an ndarray, there should be no need for these explicit ndarray checks. So eliminate them by using ensure_ndarray to get an ndarray object instead.

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
Docs build locally (e.g., run tox -e docs)
AppVeyor and Travis CI passes
Test coverage is 100% (Coveralls passes)

Instead of checking to see if `chunk` is an `ndarray`, use `ensure_ndarray` to get an `ndarray` viewing the underlying data in `chunk`. This way we know attributes like `dtype` are available and can be checked easily. Also makes this a bit more friendly with other array-like types.

Since we are interested in getting an `ndarray` representing the buffer within `out` for writing into, go ahead and use `ensure_ndarray` to coerce the underlying buffer into an `ndarray`. This way we can avoid a needless check and just write into any array-like value for `out` that is provided.

Appears that `out` can also be a Zarr `Array` or any other array-like that does not expose a buffer, but does allow writing into. In these cases `ensure_ndarray` will fail as there is not an underlying buffer that can be used with an `ndarray`. To also handle this case, catch the `TypeError` that `ensure_ndarray` will raise in this case and use that to indicate whether `out` is now an `ndarray` or not. This allows us to continue to write into arbitrary buffers, but also correctly handle objects that do not expose buffers.

jakirkham · 2019-11-11T21:57:38Z

@jrbourbeau, if you are around, this could use a review 🙂

jakirkham · 2019-11-11T23:08:22Z


+ out_is_ndarray = True
+ try:
+ out = ensure_ndarray(out)


Note that this is already checked by test_get_selection_out. Without this try/except, we get the following TypeError because out is a Zarr Array, which cannot be coerced to an ndarray. This is ok and intentional. So we just carry on without coercing that case and note that we do not have an ndarray when checking later.
Details
_________________________test_get_selection_out____________________________deftest_get_selection_out(): # basic selectionsa=np.arange(1050) z=zarr.create(shape=1050, chunks=100, dtype=a.dtype) z[:] =aselections= [ slice(50, 150), slice(0, 1050), slice(1, 2), ] forselectioninselections: expect=a[selection] out=zarr.create(shape=expect.shape, chunks=10, dtype=expect.dtype, fill_value=0) >z.get_basic_selection(selection, out=out) zarr/tests/test_indexing.py:1036: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:698: inget_basic_selectionfields=fields) zarr/core.py:740: in_get_basic_selection_ndreturnself._get_selection(indexer=indexer, out=out, fields=fields) zarr/core.py:1028: in_get_selectiondrop_axes=indexer.drop_axes, fields=fields) zarr/core.py:1573: in_chunk_getitemout=ensure_ndarray(out) __ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ buf=<zarr.core.Array (100,) int64>defensure_ndarray(buf): """Convenience function to coerce `buf` to a numpy array, if it is not already a numpy array. Parameters ---------- buf : array-like or bytes-like A numpy array or any object exporting a buffer interface. Returns ------- arr : ndarray A numpy array, sharing memory with `buf`. Notes ----- This function will not create a copy under any circumstances, it is guaranteed to return a view on memory exported by `buf`. """ifisinstance(buf, np.ndarray): # already a numpy arrayarr=bufelifisinstance(buf, array.array) andbuf.typecodein'cu': # Guard condition, do not support array.array with unicode type, this is# problematic because numpy does not support it on all platforms. Also do not# support char as it was removed in Python 3.raiseTypeError('array.array with char or unicode type is not supported') else: # N.B., first take a memoryview to make sure that we subsequently create a# numpy array from a memory buffer with no copyifPY2: # pragma: py3 no covertry: mem=memoryview(buf) exceptTypeError: # on PY2 also check if object exports old-style buffer interfacemem=np.getbuffer(buf) else: # pragma: py2 no cover>mem=memoryview(buf) ETypeError: memoryview: abytes-likeobjectisrequired, not'Array' .tox/py36/lib/python3.6/site-packages/numcodecs/compat.py:74: TypeError
ref: https://travis-ci.org/zarr-developers/zarr-python/jobs/610533967#L2316

jakirkham · 2019-11-12T02:00:21Z

Planning on merging EOD tomorrow if no comments.

jrbourbeau

Thanks for the PR @jakirkham! Generally these changes seem fine by me. I have one question about when ensure_ndarray raises an error that I've left below.

jrbourbeau · 2019-11-12T02:18:42Z


 # check object encoding
- if isinstance(chunk, np.ndarray) and chunk.dtype == object:
+ if ensure_ndarray(chunk).dtype == object:


Is it possible that some non-ndarrays would previously skip over this if block but now fail if ensure_ndarray raises an error? For example:
In [28]: fromnumcodecs.compatimportensure_ndarrayIn [29]: importarrayIn [30]: chunk=array.array('u', 'hello \u2641') In [31]: ifisinstance(chunk, np.ndarray) andchunk.dtype==object: ...: pass ...: In [32]: ifensure_ndarray(chunk).dtype==object: ...: pass ...: ---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast) <ipython-input-32-e1e15cbf81c2>in<module>---->1ifensure_ndarray(chunk).dtype==object: 2pass3~/miniconda/envs/zarr-python-dev/lib/python3.7/site-packages/numcodecs/compat.pyinensure_ndarray(buf) 63# problematic because numpy does not support it on all platforms. Also do not64# support char as it was removed in Python 3.--->65raiseTypeError('array.array with char or unicode type is not supported') 6667else: TypeError: array.arraywithcharorunicodetypeisnotsupported
To be clear, I'm not sure how likely this is (or if it's even possible) to come up in practice. Do you have a sense for this?

That exception would be expected as we have decided not to work with Python builtin arrays that use character or unicode types. There is some more detailed discussion in this thread.

Basically the only exception that we need to worry about is the one coming from memoryview, which will be a TypeError if the object doesn't support the buffer protocol. After that point we have a memoryview. So we know the rest of the code will work (unless NumPy gets a bug ;).
Edit: Sorry was looking at the other change for a second.

For context this function is trying to turn some data into bytes that can be serialized (say to a file on disk).
The filters before this step should be returning something that either is an ndarray or could be coerced to one. This is required by the compressors that follow and the storage layer afterwards. So if ensure_ndarray fails here, then we have invalid data and raising an exception to the user would be appropriate.

…y_more

jrbourbeau

Great, thank you for providing all the additional context surrounding this section. I pushed a commit with a changelog entry. Feel free to merge on green. Thanks @jakirkham!

jakirkham · 2019-11-12T18:56:28Z

Thanks @jrbourbeau for the review and fix! 😄

jakirkham · 2019-11-11T19:48:53Z

As we can coerce any object that is array-like to an ndarray, there should be no need for these explicit ndarray checks. So eliminate them by using ensure_ndarray to get an ndarray object instead.

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
Docs build locally (e.g., run tox -e docs)
AppVeyor and Travis CI passes
Test coverage is 100% (Coveralls passes)

Instead of checking to see if `chunk` is an `ndarray`, use `ensure_ndarray` to get an `ndarray` viewing the underlying data in `chunk`. This way we know attributes like `dtype` are available and can be checked easily. Also makes this a bit more friendly with other array-like types.

Since we are interested in getting an `ndarray` representing the buffer within `out` for writing into, go ahead and use `ensure_ndarray` to coerce the underlying buffer into an `ndarray`. This way we can avoid a needless check and just write into any array-like value for `out` that is provided.

Appears that `out` can also be a Zarr `Array` or any other array-like that does not expose a buffer, but does allow writing into. In these cases `ensure_ndarray` will fail as there is not an underlying buffer that can be used with an `ndarray`. To also handle this case, catch the `TypeError` that `ensure_ndarray` will raise in this case and use that to indicate whether `out` is now an `ndarray` or not. This allows us to continue to write into arbitrary buffers, but also correctly handle objects that do not expose buffers.

jakirkham · 2019-11-11T21:57:38Z

@jrbourbeau, if you are around, this could use a review 🙂

jakirkham · 2019-11-11T23:08:22Z


+ out_is_ndarray = True
+ try:
+ out = ensure_ndarray(out)


Note that this is already checked by test_get_selection_out. Without this try/except, we get the following TypeError because out is a Zarr Array, which cannot be coerced to an ndarray. This is ok and intentional. So we just carry on without coercing that case and note that we do not have an ndarray when checking later.
Details
_________________________test_get_selection_out____________________________deftest_get_selection_out(): # basic selectionsa=np.arange(1050) z=zarr.create(shape=1050, chunks=100, dtype=a.dtype) z[:] =aselections= [ slice(50, 150), slice(0, 1050), slice(1, 2), ] forselectioninselections: expect=a[selection] out=zarr.create(shape=expect.shape, chunks=10, dtype=expect.dtype, fill_value=0) >z.get_basic_selection(selection, out=out) zarr/tests/test_indexing.py:1036: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:698: inget_basic_selectionfields=fields) zarr/core.py:740: in_get_basic_selection_ndreturnself._get_selection(indexer=indexer, out=out, fields=fields) zarr/core.py:1028: in_get_selectiondrop_axes=indexer.drop_axes, fields=fields) zarr/core.py:1573: in_chunk_getitemout=ensure_ndarray(out) __ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ buf=<zarr.core.Array (100,) int64>defensure_ndarray(buf): """Convenience function to coerce `buf` to a numpy array, if it is not already a numpy array. Parameters ---------- buf : array-like or bytes-like A numpy array or any object exporting a buffer interface. Returns ------- arr : ndarray A numpy array, sharing memory with `buf`. Notes ----- This function will not create a copy under any circumstances, it is guaranteed to return a view on memory exported by `buf`. """ifisinstance(buf, np.ndarray): # already a numpy arrayarr=bufelifisinstance(buf, array.array) andbuf.typecodein'cu': # Guard condition, do not support array.array with unicode type, this is# problematic because numpy does not support it on all platforms. Also do not# support char as it was removed in Python 3.raiseTypeError('array.array with char or unicode type is not supported') else: # N.B., first take a memoryview to make sure that we subsequently create a# numpy array from a memory buffer with no copyifPY2: # pragma: py3 no covertry: mem=memoryview(buf) exceptTypeError: # on PY2 also check if object exports old-style buffer interfacemem=np.getbuffer(buf) else: # pragma: py2 no cover>mem=memoryview(buf) ETypeError: memoryview: abytes-likeobjectisrequired, not'Array' .tox/py36/lib/python3.6/site-packages/numcodecs/compat.py:74: TypeError
ref: https://travis-ci.org/zarr-developers/zarr-python/jobs/610533967#L2316

jakirkham · 2019-11-12T02:00:21Z

Planning on merging EOD tomorrow if no comments.

jrbourbeau

Thanks for the PR @jakirkham! Generally these changes seem fine by me. I have one question about when ensure_ndarray raises an error that I've left below.

jrbourbeau · 2019-11-12T02:18:42Z


 # check object encoding
- if isinstance(chunk, np.ndarray) and chunk.dtype == object:
+ if ensure_ndarray(chunk).dtype == object:


Is it possible that some non-ndarrays would previously skip over this if block but now fail if ensure_ndarray raises an error? For example:
In [28]: fromnumcodecs.compatimportensure_ndarrayIn [29]: importarrayIn [30]: chunk=array.array('u', 'hello \u2641') In [31]: ifisinstance(chunk, np.ndarray) andchunk.dtype==object: ...: pass ...: In [32]: ifensure_ndarray(chunk).dtype==object: ...: pass ...: ---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast) <ipython-input-32-e1e15cbf81c2>in<module>---->1ifensure_ndarray(chunk).dtype==object: 2pass3~/miniconda/envs/zarr-python-dev/lib/python3.7/site-packages/numcodecs/compat.pyinensure_ndarray(buf) 63# problematic because numpy does not support it on all platforms. Also do not64# support char as it was removed in Python 3.--->65raiseTypeError('array.array with char or unicode type is not supported') 6667else: TypeError: array.arraywithcharorunicodetypeisnotsupported
To be clear, I'm not sure how likely this is (or if it's even possible) to come up in practice. Do you have a sense for this?

That exception would be expected as we have decided not to work with Python builtin arrays that use character or unicode types. There is some more detailed discussion in this thread.

Basically the only exception that we need to worry about is the one coming from memoryview, which will be a TypeError if the object doesn't support the buffer protocol. After that point we have a memoryview. So we know the rest of the code will work (unless NumPy gets a bug ;).
Edit: Sorry was looking at the other change for a second.

For context this function is trying to turn some data into bytes that can be serialized (say to a file on disk).
The filters before this step should be returning something that either is an ndarray or could be coerced to one. This is required by the compressors that follow and the storage layer afterwards. So if ensure_ndarray fails here, then we have invalid data and raising an exception to the user would be appropriate.

…y_more

jrbourbeau

Great, thank you for providing all the additional context surrounding this section. I pushed a commit with a changelog entry. Feel free to merge on green. Thanks @jakirkham!

jakirkham · 2019-11-12T18:56:28Z

Thanks @jrbourbeau for the review and fix! 😄

jakirkham · 2019-11-11T19:48:53Z

As we can coerce any object that is array-like to an ndarray, there should be no need for these explicit ndarray checks. So eliminate them by using ensure_ndarray to get an ndarray object instead.

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
Docs build locally (e.g., run tox -e docs)
AppVeyor and Travis CI passes
Test coverage is 100% (Coveralls passes)

Instead of checking to see if `chunk` is an `ndarray`, use `ensure_ndarray` to get an `ndarray` viewing the underlying data in `chunk`. This way we know attributes like `dtype` are available and can be checked easily. Also makes this a bit more friendly with other array-like types.

Since we are interested in getting an `ndarray` representing the buffer within `out` for writing into, go ahead and use `ensure_ndarray` to coerce the underlying buffer into an `ndarray`. This way we can avoid a needless check and just write into any array-like value for `out` that is provided.

Appears that `out` can also be a Zarr `Array` or any other array-like that does not expose a buffer, but does allow writing into. In these cases `ensure_ndarray` will fail as there is not an underlying buffer that can be used with an `ndarray`. To also handle this case, catch the `TypeError` that `ensure_ndarray` will raise in this case and use that to indicate whether `out` is now an `ndarray` or not. This allows us to continue to write into arbitrary buffers, but also correctly handle objects that do not expose buffers.

jakirkham · 2019-11-11T21:57:38Z

@jrbourbeau, if you are around, this could use a review 🙂

jakirkham · 2019-11-11T23:08:22Z


+ out_is_ndarray = True
+ try:
+ out = ensure_ndarray(out)


Note that this is already checked by test_get_selection_out. Without this try/except, we get the following TypeError because out is a Zarr Array, which cannot be coerced to an ndarray. This is ok and intentional. So we just carry on without coercing that case and note that we do not have an ndarray when checking later.
Details
_________________________test_get_selection_out____________________________deftest_get_selection_out(): # basic selectionsa=np.arange(1050) z=zarr.create(shape=1050, chunks=100, dtype=a.dtype) z[:] =aselections= [ slice(50, 150), slice(0, 1050), slice(1, 2), ] forselectioninselections: expect=a[selection] out=zarr.create(shape=expect.shape, chunks=10, dtype=expect.dtype, fill_value=0) >z.get_basic_selection(selection, out=out) zarr/tests/test_indexing.py:1036: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:698: inget_basic_selectionfields=fields) zarr/core.py:740: in_get_basic_selection_ndreturnself._get_selection(indexer=indexer, out=out, fields=fields) zarr/core.py:1028: in_get_selectiondrop_axes=indexer.drop_axes, fields=fields) zarr/core.py:1573: in_chunk_getitemout=ensure_ndarray(out) __ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ buf=<zarr.core.Array (100,) int64>defensure_ndarray(buf): """Convenience function to coerce `buf` to a numpy array, if it is not already a numpy array. Parameters ---------- buf : array-like or bytes-like A numpy array or any object exporting a buffer interface. Returns ------- arr : ndarray A numpy array, sharing memory with `buf`. Notes ----- This function will not create a copy under any circumstances, it is guaranteed to return a view on memory exported by `buf`. """ifisinstance(buf, np.ndarray): # already a numpy arrayarr=bufelifisinstance(buf, array.array) andbuf.typecodein'cu': # Guard condition, do not support array.array with unicode type, this is# problematic because numpy does not support it on all platforms. Also do not# support char as it was removed in Python 3.raiseTypeError('array.array with char or unicode type is not supported') else: # N.B., first take a memoryview to make sure that we subsequently create a# numpy array from a memory buffer with no copyifPY2: # pragma: py3 no covertry: mem=memoryview(buf) exceptTypeError: # on PY2 also check if object exports old-style buffer interfacemem=np.getbuffer(buf) else: # pragma: py2 no cover>mem=memoryview(buf) ETypeError: memoryview: abytes-likeobjectisrequired, not'Array' .tox/py36/lib/python3.6/site-packages/numcodecs/compat.py:74: TypeError
ref: https://travis-ci.org/zarr-developers/zarr-python/jobs/610533967#L2316

jakirkham · 2019-11-12T02:00:21Z

Planning on merging EOD tomorrow if no comments.

jrbourbeau

Thanks for the PR @jakirkham! Generally these changes seem fine by me. I have one question about when ensure_ndarray raises an error that I've left below.

jrbourbeau · 2019-11-12T02:18:42Z


 # check object encoding
- if isinstance(chunk, np.ndarray) and chunk.dtype == object:
+ if ensure_ndarray(chunk).dtype == object:


Is it possible that some non-ndarrays would previously skip over this if block but now fail if ensure_ndarray raises an error? For example:
In [28]: fromnumcodecs.compatimportensure_ndarrayIn [29]: importarrayIn [30]: chunk=array.array('u', 'hello \u2641') In [31]: ifisinstance(chunk, np.ndarray) andchunk.dtype==object: ...: pass ...: In [32]: ifensure_ndarray(chunk).dtype==object: ...: pass ...: ---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast) <ipython-input-32-e1e15cbf81c2>in<module>---->1ifensure_ndarray(chunk).dtype==object: 2pass3~/miniconda/envs/zarr-python-dev/lib/python3.7/site-packages/numcodecs/compat.pyinensure_ndarray(buf) 63# problematic because numpy does not support it on all platforms. Also do not64# support char as it was removed in Python 3.--->65raiseTypeError('array.array with char or unicode type is not supported') 6667else: TypeError: array.arraywithcharorunicodetypeisnotsupported
To be clear, I'm not sure how likely this is (or if it's even possible) to come up in practice. Do you have a sense for this?

That exception would be expected as we have decided not to work with Python builtin arrays that use character or unicode types. There is some more detailed discussion in this thread.

Basically the only exception that we need to worry about is the one coming from memoryview, which will be a TypeError if the object doesn't support the buffer protocol. After that point we have a memoryview. So we know the rest of the code will work (unless NumPy gets a bug ;).
Edit: Sorry was looking at the other change for a second.

For context this function is trying to turn some data into bytes that can be serialized (say to a file on disk).
The filters before this step should be returning something that either is an ndarray or could be coerced to one. This is required by the compressors that follow and the storage layer afterwards. So if ensure_ndarray fails here, then we have invalid data and raising an exception to the user would be appropriate.

…y_more

jrbourbeau

Great, thank you for providing all the additional context surrounding this section. I pushed a commit with a changelog entry. Feel free to merge on green. Thanks @jakirkham!

jakirkham · 2019-11-12T18:56:28Z

Thanks @jrbourbeau for the review and fix! 😄

jakirkham · 2019-11-11T19:48:53Z

As we can coerce any object that is array-like to an ndarray, there should be no need for these explicit ndarray checks. So eliminate them by using ensure_ndarray to get an ndarray object instead.

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
Docs build locally (e.g., run tox -e docs)
AppVeyor and Travis CI passes
Test coverage is 100% (Coveralls passes)

Instead of checking to see if `chunk` is an `ndarray`, use `ensure_ndarray` to get an `ndarray` viewing the underlying data in `chunk`. This way we know attributes like `dtype` are available and can be checked easily. Also makes this a bit more friendly with other array-like types.

Since we are interested in getting an `ndarray` representing the buffer within `out` for writing into, go ahead and use `ensure_ndarray` to coerce the underlying buffer into an `ndarray`. This way we can avoid a needless check and just write into any array-like value for `out` that is provided.

Appears that `out` can also be a Zarr `Array` or any other array-like that does not expose a buffer, but does allow writing into. In these cases `ensure_ndarray` will fail as there is not an underlying buffer that can be used with an `ndarray`. To also handle this case, catch the `TypeError` that `ensure_ndarray` will raise in this case and use that to indicate whether `out` is now an `ndarray` or not. This allows us to continue to write into arbitrary buffers, but also correctly handle objects that do not expose buffers.

jakirkham · 2019-11-11T21:57:38Z

@jrbourbeau, if you are around, this could use a review 🙂

jakirkham · 2019-11-11T23:08:22Z


+ out_is_ndarray = True
+ try:
+ out = ensure_ndarray(out)


Note that this is already checked by test_get_selection_out. Without this try/except, we get the following TypeError because out is a Zarr Array, which cannot be coerced to an ndarray. This is ok and intentional. So we just carry on without coercing that case and note that we do not have an ndarray when checking later.
Details
_________________________test_get_selection_out____________________________deftest_get_selection_out(): # basic selectionsa=np.arange(1050) z=zarr.create(shape=1050, chunks=100, dtype=a.dtype) z[:] =aselections= [ slice(50, 150), slice(0, 1050), slice(1, 2), ] forselectioninselections: expect=a[selection] out=zarr.create(shape=expect.shape, chunks=10, dtype=expect.dtype, fill_value=0) >z.get_basic_selection(selection, out=out) zarr/tests/test_indexing.py:1036: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:698: inget_basic_selectionfields=fields) zarr/core.py:740: in_get_basic_selection_ndreturnself._get_selection(indexer=indexer, out=out, fields=fields) zarr/core.py:1028: in_get_selectiondrop_axes=indexer.drop_axes, fields=fields) zarr/core.py:1573: in_chunk_getitemout=ensure_ndarray(out) __ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ buf=<zarr.core.Array (100,) int64>defensure_ndarray(buf): """Convenience function to coerce `buf` to a numpy array, if it is not already a numpy array. Parameters ---------- buf : array-like or bytes-like A numpy array or any object exporting a buffer interface. Returns ------- arr : ndarray A numpy array, sharing memory with `buf`. Notes ----- This function will not create a copy under any circumstances, it is guaranteed to return a view on memory exported by `buf`. """ifisinstance(buf, np.ndarray): # already a numpy arrayarr=bufelifisinstance(buf, array.array) andbuf.typecodein'cu': # Guard condition, do not support array.array with unicode type, this is# problematic because numpy does not support it on all platforms. Also do not# support char as it was removed in Python 3.raiseTypeError('array.array with char or unicode type is not supported') else: # N.B., first take a memoryview to make sure that we subsequently create a# numpy array from a memory buffer with no copyifPY2: # pragma: py3 no covertry: mem=memoryview(buf) exceptTypeError: # on PY2 also check if object exports old-style buffer interfacemem=np.getbuffer(buf) else: # pragma: py2 no cover>mem=memoryview(buf) ETypeError: memoryview: abytes-likeobjectisrequired, not'Array' .tox/py36/lib/python3.6/site-packages/numcodecs/compat.py:74: TypeError
ref: https://travis-ci.org/zarr-developers/zarr-python/jobs/610533967#L2316

jakirkham · 2019-11-12T02:00:21Z

Planning on merging EOD tomorrow if no comments.

jrbourbeau

Thanks for the PR @jakirkham! Generally these changes seem fine by me. I have one question about when ensure_ndarray raises an error that I've left below.

jrbourbeau · 2019-11-12T02:18:42Z


 # check object encoding
- if isinstance(chunk, np.ndarray) and chunk.dtype == object:
+ if ensure_ndarray(chunk).dtype == object:


Is it possible that some non-ndarrays would previously skip over this if block but now fail if ensure_ndarray raises an error? For example:
In [28]: fromnumcodecs.compatimportensure_ndarrayIn [29]: importarrayIn [30]: chunk=array.array('u', 'hello \u2641') In [31]: ifisinstance(chunk, np.ndarray) andchunk.dtype==object: ...: pass ...: In [32]: ifensure_ndarray(chunk).dtype==object: ...: pass ...: ---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast) <ipython-input-32-e1e15cbf81c2>in<module>---->1ifensure_ndarray(chunk).dtype==object: 2pass3~/miniconda/envs/zarr-python-dev/lib/python3.7/site-packages/numcodecs/compat.pyinensure_ndarray(buf) 63# problematic because numpy does not support it on all platforms. Also do not64# support char as it was removed in Python 3.--->65raiseTypeError('array.array with char or unicode type is not supported') 6667else: TypeError: array.arraywithcharorunicodetypeisnotsupported
To be clear, I'm not sure how likely this is (or if it's even possible) to come up in practice. Do you have a sense for this?

That exception would be expected as we have decided not to work with Python builtin arrays that use character or unicode types. There is some more detailed discussion in this thread.

Basically the only exception that we need to worry about is the one coming from memoryview, which will be a TypeError if the object doesn't support the buffer protocol. After that point we have a memoryview. So we know the rest of the code will work (unless NumPy gets a bug ;).
Edit: Sorry was looking at the other change for a second.

For context this function is trying to turn some data into bytes that can be serialized (say to a file on disk).
The filters before this step should be returning something that either is an ndarray or could be coerced to one. This is required by the compressors that follow and the storage layer afterwards. So if ensure_ndarray fails here, then we have invalid data and raising an exception to the user would be appropriate.

…y_more

jrbourbeau

Great, thank you for providing all the additional context surrounding this section. I pushed a commit with a changelog entry. Feel free to merge on green. Thanks @jakirkham!

jakirkham · 2019-11-12T18:56:28Z

Thanks @jrbourbeau for the review and fix! 😄

jakirkham added 3 commits November 11, 2019 15:05

jakirkham commented Nov 11, 2019
View reviewed changes

jrbourbeau reviewed Nov 12, 2019
View reviewed changes

jrbourbeau added 2 commits November 12, 2019 11:16

Adds release notes entry
b222f0a

Merge remote-tracking branch 'upstream/master' into use_ensure_ndarra…
52205af
…y_more

jrbourbeau approved these changes Nov 12, 2019
View reviewed changes

jakirkham merged commit 58b1786 into zarr-developers:masterNov 12, 2019

jakirkham deleted the use_ensure_ndarray_more branch November 12, 2019 18:56

Carreau added this to the v2.4 milestone Sep 9, 2020

jakirkham added 3 commits November 11, 2019 15:05

jakirkham commented Nov 11, 2019
View reviewed changes

jrbourbeau reviewed Nov 12, 2019
View reviewed changes

jrbourbeau added 2 commits November 12, 2019 11:16

Adds release notes entry
b222f0a

Merge remote-tracking branch 'upstream/master' into use_ensure_ndarra…
52205af
…y_more

jrbourbeau approved these changes Nov 12, 2019
View reviewed changes

jakirkham merged commit 58b1786 into zarr-developers:masterNov 12, 2019

jakirkham deleted the use_ensure_ndarray_more branch November 12, 2019 18:56

Carreau added this to the v2.4 milestone Sep 9, 2020

jakirkham added 3 commits November 11, 2019 15:05

jakirkham commented Nov 11, 2019
View reviewed changes

jrbourbeau reviewed Nov 12, 2019
View reviewed changes

jrbourbeau added 2 commits November 12, 2019 11:16

Adds release notes entry
b222f0a

Merge remote-tracking branch 'upstream/master' into use_ensure_ndarra…
52205af
…y_more

jrbourbeau approved these changes Nov 12, 2019
View reviewed changes

jakirkham merged commit 58b1786 into zarr-developers:masterNov 12, 2019

jakirkham deleted the use_ensure_ndarray_more branch November 12, 2019 18:56

Carreau added this to the v2.4 milestone Sep 9, 2020

jakirkham added 3 commits November 11, 2019 15:05

jakirkham commented Nov 11, 2019
View reviewed changes

jrbourbeau reviewed Nov 12, 2019
View reviewed changes

jrbourbeau added 2 commits November 12, 2019 11:16

Adds release notes entry
b222f0a

Merge remote-tracking branch 'upstream/master' into use_ensure_ndarra…
52205af
…y_more

jrbourbeau approved these changes Nov 12, 2019
View reviewed changes

jakirkham merged commit 58b1786 into zarr-developers:masterNov 12, 2019

jakirkham deleted the use_ensure_ndarray_more branch November 12, 2019 18:56

Carreau added this to the v2.4 milestone Sep 9, 2020

jakirkham added 3 commits November 11, 2019 15:05

jakirkham commented Nov 11, 2019
View reviewed changes

jrbourbeau reviewed Nov 12, 2019
View reviewed changes

jrbourbeau added 2 commits November 12, 2019 11:16

Adds release notes entry
b222f0a

Merge remote-tracking branch 'upstream/master' into use_ensure_ndarra…
52205af
…y_more

jrbourbeau approved these changes Nov 12, 2019
View reviewed changes

jakirkham merged commit 58b1786 into zarr-developers:masterNov 12, 2019

jakirkham deleted the use_ensure_ndarray_more branch November 12, 2019 18:56

Carreau added this to the v2.4 milestone Sep 9, 2020

jakirkham added 3 commits November 11, 2019 15:05

jakirkham commented Nov 11, 2019
View reviewed changes

jrbourbeau reviewed Nov 12, 2019
View reviewed changes

jrbourbeau added 2 commits November 12, 2019 11:16

Adds release notes entry
b222f0a

Merge remote-tracking branch 'upstream/master' into use_ensure_ndarra…
52205af
…y_more

jrbourbeau approved these changes Nov 12, 2019
View reviewed changes

jakirkham merged commit 58b1786 into zarr-developers:masterNov 12, 2019

jakirkham deleted the use_ensure_ndarray_more branch November 12, 2019 18:56

Carreau added this to the v2.4 milestone Sep 9, 2020

jakirkham added 3 commits November 11, 2019 15:05

jakirkham commented Nov 11, 2019
View reviewed changes

jrbourbeau reviewed Nov 12, 2019
View reviewed changes

jrbourbeau added 2 commits November 12, 2019 11:16

Adds release notes entry
b222f0a

Merge remote-tracking branch 'upstream/master' into use_ensure_ndarra…
52205af
…y_more

jrbourbeau approved these changes Nov 12, 2019
View reviewed changes

jakirkham merged commit 58b1786 into zarr-developers:masterNov 12, 2019

jakirkham deleted the use_ensure_ndarray_more branch November 12, 2019 18:56

Carreau added this to the v2.4 milestone Sep 9, 2020

jakirkham added 3 commits November 11, 2019 15:05

jakirkham commented Nov 11, 2019
View reviewed changes

jrbourbeau reviewed Nov 12, 2019
View reviewed changes

jrbourbeau added 2 commits November 12, 2019 11:16

Adds release notes entry
b222f0a

Merge remote-tracking branch 'upstream/master' into use_ensure_ndarra…
52205af
…y_more

jrbourbeau approved these changes Nov 12, 2019
View reviewed changes

jakirkham merged commit 58b1786 into zarr-developers:masterNov 12, 2019

jakirkham deleted the use_ensure_ndarray_more branch November 12, 2019 18:56

Carreau added this to the v2.4 milestone Sep 9, 2020

Uh oh!

Uh oh!

Conversation

jakirkham commented Nov 11, 2019

Uh oh!

jakirkham commented Nov 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakirkham commented Nov 12, 2019

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakirkhamNov 12, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

jakirkham commented Nov 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

jakirkham commented Nov 11, 2019

Uh oh!

jakirkham commented Nov 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakirkham commented Nov 12, 2019

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakirkhamNov 12, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

jakirkham commented Nov 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

jakirkham commented Nov 11, 2019

Uh oh!

jakirkham commented Nov 11, 2019

jakirkhamNov 12, 2019•
edited
Loading

jakirkhamNov 12, 2019•
edited
Loading

jakirkhamNov 12, 2019•
edited
Loading