Skip to content
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

Python: Remove numpy requirement, use buffer protocol instead #615

Merged
merged 4 commits into from Sep 14, 2023

Conversation

EchterAgo
Copy link
Contributor

@EchterAgo EchterAgo commented Sep 8, 2023

Partly taken from #283 (comment)

This removes the requirement of read_barcode / write_barcode to have Numpy installed and uses buffer protocol instead.

Numpy arrays can already be converted to a buffer, for PIL images we get the __array_interface__ and use its values to create a memory view with the right shape.

The write_barcode function now returns a buffer instead of a Numpy array. This buffer also supports __array_interface__ for easy conversion to PIL images or Numpy arrays.

  • For PIL: img = Image.fromarray(result, "L")
  • For Numpy: img = np.array(result)

fixes #283

Co-authored-by: axxel

TODO:

  • Rebase
  • Add @axxel as co-author to the commit
  • Use Reduce from ZXAlgorithms.h
  • Remove the TODO in write_barcode, returning a buffer is OK here
  • Clean up code
  • Investigate whether we can return WriteResult or py::buffer directly from write_barcode
  • Review for memory safety
  • Do we need to adjust any documentation?

@EchterAgo EchterAgo marked this pull request as draft September 8, 2023 10:41
@EchterAgo

This comment was marked as outdated.

@EchterAgo EchterAgo force-pushed the remove_numpy branch 3 times, most recently from acd7a0b to da69908 Compare September 10, 2023 06:08
@EchterAgo EchterAgo marked this pull request as ready for review September 10, 2023 06:12
@EchterAgo
Copy link
Contributor Author

I fixed the tests, TestReadWrite now runs without numpy, I added additional tests that test buffer protocol and when numpy is installed we also test numpy array conversion. One issue I see right now is that the zxingcpp.write_barcode return type changed from numpy array to a buffer. We might be able to return a type that has __array_interface__ here.

@EchterAgo

This comment was marked as resolved.

@EchterAgo
Copy link
Contributor Author

EchterAgo commented Sep 10, 2023

Updated for much better __array_interface__ compatibility from any type implementing it. The normal __array_interface__ is passing a tuple for data that has a pointer and a readonly flag, but PIL for example only passes/expects byte/buffer objects there. In write_barcode we also pass a buffer in data instead of a pointer, which is something I want to change. data seems to also be optional, so I think we could leave it out.

This still needs to be cleaned up a bit and I'm not sure it is worth adding over just the standard buffer protocol support.

Copy link
Collaborator

@axxel axxel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this and bringing us closer to getting rid of the numpy dependency. I like the array_interface approach but if I understand this correct, we might still have an API breakage issue, right? Do you know if there is something like an implicit casting on the Python side happening when someone would try to pass the returned object to a function that takes a numpy array as parameter?

wrappers/python/zxing.cpp Show resolved Hide resolved
wrappers/python/zxing.cpp Outdated Show resolved Hide resolved
wrappers/python/zxing.cpp Show resolved Hide resolved
wrappers/python/zxing.cpp Outdated Show resolved Hide resolved
wrappers/python/zxing.cpp Outdated Show resolved Hide resolved
wrappers/python/zxing.cpp Outdated Show resolved Hide resolved
@EchterAgo

This comment was marked as resolved.

@EchterAgo
Copy link
Contributor Author

EchterAgo commented Sep 10, 2023

Thanks for working on this and bringing us closer to getting rid of the numpy dependency. I like the array_interface approach but if I understand this correct, we might still have an API breakage issue, right? Do you know if there is something like an implicit casting on the Python side happening when someone would try to pass the returned object to a function that takes a numpy array as parameter?

Yes, this breaks the API. Sadly there is no implicit cast:

>>> data = b'abcdefg'
>>> import numpy as np
>>> mvdata = memoryview(data)
>>> np.array(mvdata)
array([ 97,  98,  99, 100, 101, 102, 103], dtype=uint8)
>>> def test(a: np.array):
...    print(a)
... 
>>> test(mvdata)
<memory at 0x00000205FF1AD6C0>

This still works when passing it to other libraries that handle buffer protocol or array interface internally, in some code it would need an explicit construction of a numpy array, PIL Image or similar though.

@EchterAgo

This comment was marked as resolved.

@EchterAgo EchterAgo force-pushed the remove_numpy branch 3 times, most recently from b1a81d7 to c45f74a Compare September 14, 2023 07:25
Partly taken from zxing-cpp#283 (comment)

This removes the requirement of `read_barcode` / `write_barcode` to have
Numpy installed and uses [buffer protocol](https://pybind11.readthedocs.io/en/stable/advanced/pycpp/numpy.html#buffer-protocol)
instead.

Numpy arrays can already be converted to a buffer, for PIL images we
get the `__array_interface__` and use its values to create a memory view
with the right shape.

The `write_barcode` function now returns a buffer instead of a Numpy
array. This buffer also supports `__array_interface__` for easy
conversion to PIL images or Numpy arrays.

* For PIL: `img = Image.fromarray(result, "L")`
* For Numpy: `img = np.array(result)`

fixes zxing-cpp#283

Co-authored-by: axxel <awagger@gmail.com>
@EchterAgo
Copy link
Contributor Author

I don't really understand Cython enough to do a proper test with it yet, so I removed it from the TODO.

@EchterAgo
Copy link
Contributor Author

If there is no documentation to adjust, I think this is ready.

@EchterAgo
Copy link
Contributor Author

I checked README.md, demo_reader.py and demo_writer.py, everything looks good to me.

@axxel
Copy link
Collaborator

axxel commented Sep 14, 2023

Will squash and merge once the CI checks are done. Vielen Dank Axel.

@EchterAgo
Copy link
Contributor Author

Super, vielen Dank auch an dich, Axel!

@axxel axxel merged commit b71358a into zxing-cpp:master Sep 14, 2023
16 checks passed
@EchterAgo EchterAgo deleted the remove_numpy branch September 14, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python bindings without numpy?
2 participants