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 Bindings didn't allow for zero-D Funcs, ImageParams, Buffers #6633

Merged
merged 2 commits into from
Mar 4, 2022

Conversation

steven-johnson
Copy link
Contributor

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

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
Copy link
Member

abadams commented Mar 3, 2022

An alternative to None might be the empty tuple: [()]

@alexreinking
Copy link
Member

alexreinking commented Mar 3, 2022

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

@steven-johnson
Copy link
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
Contributor Author

Final votes on syntax?

@alexreinking
Copy link
Member

alexreinking commented Mar 3, 2022

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
Contributor Author

PTAL

@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