Python Bindings didn't allow for zero-D Funcs, ImageParams, Buffers by steven-johnson · Pull Request #6633 · halide/Halide · GitHub
Skip to content

Python Bindings didn't allow for zero-D Funcs, ImageParams, Buffers#6633

Merged
steven-johnson merged 2 commits into
mainfrom
srj/pybind-scalar-funcs
Mar 4, 2022
Merged

Python Bindings didn't allow for zero-D Funcs, ImageParams, Buffers#6633
steven-johnson merged 2 commits into
mainfrom
srj/pybind-scalar-funcs

Conversation

@steven-johnson

@steven-johnson steven-johnson commented Mar 3, 2022

Copy link
Copy Markdown
Contributor

There were no overloads or tests for accessing the element of any of these in the zero-D case, and the obvious syntax ([], to mirror C++ () in these cases) isn't legal in Python. To support this uncommon-but-necessary case, I'm proposing that we use the syntax [None], [()], which isn't pretty, but is less bad than other options I've considered so far. (Suggestions welcome.)

There were no overloads or tests for accessing the element of any of these in the zero-D case, and the obvious syntax (`[]`, to mirror C++ `()` in these cases) isn't legal in Python. To support this uncommon-but-necessary case, I'm proposing that we use the syntax `[None]`, which isn't pretty, but is less bad than other options I've considered so far. (Suggestions welcome.)
@steven-johnson steven-johnson added the release_notes For changes that may warrant a note in README for official releases. label Mar 3, 2022
@abadams

abadams commented Mar 3, 2022

Copy link
Copy Markdown
Member

@alexreinking

alexreinking commented Mar 3, 2022

Copy link
Copy Markdown
Member

None is fine. It's the most obvious thing besides the obvious, but illegal, syntax.

@steven-johnson

Copy link
Copy Markdown
Contributor Author

FWIW, [()] works even without this PR (we'd want to add the test cases, of course). I can live with either syntax.

@steven-johnson

Copy link
Copy Markdown
Contributor Author

Final votes on syntax?

@alexreinking

alexreinking commented Mar 3, 2022

Copy link
Copy Markdown
Member

Final votes on syntax?

On second thought, we shouldn't use None for this... Numpy uses it in its indexing logic to introduce a new axis of extent 1. That's not what's happening here, so to allow None in this case might well cause confusion. Let's go with the empty tuple and just make sure it's tested.

https://numpy.org/doc/stable/reference/constants.html#numpy.newaxis

@alexreinking alexreinking self-requested a review March 3, 2022 22:19
@steven-johnson

Copy link
Copy Markdown
Contributor Author

@steven-johnson steven-johnson merged commit 3827279 into main Mar 4, 2022
@steven-johnson steven-johnson deleted the srj/pybind-scalar-funcs branch March 4, 2022 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_notes For changes that may warrant a note in README for official releases.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants