Makes hl.Buffer <--> Python Buffer Protocol axis convention consistent throughout#8953
Makes hl.Buffer <--> Python Buffer Protocol axis convention consistent throughout#8953jiawen wants to merge 4 commits into
Conversation
- numpy_view() with no arguments: - Always tries to returns a C-contiguous view of the buffer if possible. - If Halide Buffer is stored in the default order, will reverse axes. - If Halide Buffer is stored in the reverse order, will preserve axes. - numpy_view(reverse_axes: Bool): - Requires an explicit reverse_axes argument to be passed in. - It will do what was requested, and supports non-contiguous buffers.
|
@alexreinking PTAL at the design. I left some |
| .def_buffer([](Buffer<> &b) -> py::buffer_info { | ||
| return to_buffer_info(b, /*reverse_axes*/ true); | ||
|
|
||
| // ELEPHANT: this always reverses axes, which might be surprising? |
There was a problem hiding this comment.
I'd like to break the interface here.
The smoothest path would be to permit this, with automatic maybe-reverse-axes-depending on whether the Buffer is contiguous, only when the Buffer is contiguous. This is analogous to the numpy_view() with no args below.
This means breaking automatic conversion from cropped Buffers to the buffer protocol - but I imagine that's not used very much. Clients can use numpy_view(reverse_axes: Bool) instead.
WDTY?
| // - It is possible for a Buffer to be both C and F contiguous (e.g., a scalar or a | ||
| // 1D vector), or for a Buffer to be neither (e.g., storage_order=[1, 0, 2] for a 3D | ||
| // buffer). | ||
| // ELEPHANT: maybe I should just call it [densest_first, densest_last]. But that |
There was a problem hiding this comment.
To discuss how to name this.
|
|
||
| // This allows us to use any buffer-like Python entity to create a Buffer<> | ||
| // (most notably, an ndarray) | ||
| .def(py::init_alias<py::buffer, const std::string &, bool>(), py::arg("buffer"), py::arg("name") = "", py::arg("reverse_axes") = true) |
There was a problem hiding this comment.
I'd like to make this more explicit as well. Have two versions:
- A version without
reverse_axesthat requires a contiguous buffer. It will auto-reverse if contiguous. - A version with explicit
reverse_axesthat accepts anything.
|
Bump |
|
@abadams @alexreinking Bump, part deux |
|
Alex and I finally spent some time talking about this. We think that conversions back and forth between numpy/python buffer protocol and Halide buffers should just always reverse the axes, and the automatic behavior in the python_extension generator output is bad. It means, for example, that calling a Halide-generated function on a numpy array computes the same thing as calling it on a copy-free transpose of the array, which seems surprising. The semantics should depend on the passed object as indexed, not as stored in memory. Having a numpy_view that doesn't reverse the axes given an explicit argument is fine too, but we think everything should default to axis reversing. |
This makes sense to me. When I find some cycles, I'll go over the API surfaces with a fine-toothed comb to make sure I catch all the "loops". Off the top of my head, the interactions are:
I'm almost tempted to make things more restrictive:
|
I don't think either of these are necessary. Just being consistent about flipping logical axes between each platform's native buffer representation should be enough. |
jiawen
changed the title
|
@alexreinking @abadams PTAL. I think this reflects the design you had in mind. I like it - the story is consistent with few surprises. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8953 +/- ##
=======================================
Coverage ? 69.37%
=======================================
Files ? 254
Lines ? 78274
Branches ? 18729
=======================================
Hits ? 54301
Misses ? 18479
Partials ? 5494 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
Halide interacts with Python in three place:
compile_to_callable()via Python bindings)This PR changes our axis ordering conventions to be consistent throughout:
hl.Bufferreverses axes.hl.Bufferfrom any Python buffer object reverses axes.hl.Bufferis also a Python buffer object. E.g., you can usemy_buffer[idx0, idx1, idx2]to retrieve an element. Constructing anhl.Bufferfrom anotherhl.Bufferdoes not reverse axes because that would be surprising.hl.Buffer(my_array, "my_buffer_name", reverse_axes=False)is explicitly supported.hl.Bufferdoes not reverse axes (it's considered a Halide Buffer, not a Python buffer object)Because of the new consistent conventions, we now permit:
Non-contiguous views. Previously, Python-side C-contiguous buffers were reversed and F-contiguous buffers were not. Now we support arbitrary views.
This includes negatives strides.
hl.Buffer also gained an explicit
.numpy_view()method that returns anp.arraywith an explicitreverse_axesargument (defaulting toTrue).