Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
steven-johnson merged 2 commits into from
Mar 4, 2022
Merged

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

steven-johnson merged 2 commits into from
Mar 4, 2022

Conversation

@steven-johnson
Copy link
Contributor

@steven-johnson steven-johnson commented Mar 3, 2022 •
edited
Loading

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
Python Bindings didn't allow for zero-D Funcs, ImageParams, Buffers
c5abb8c 
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 •
edited
Loading

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

@steven-johnson
Copy link
Contributor Author

steven-johnson commented Mar 3, 2022

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

steven-johnson commented Mar 3, 2022

Final votes on syntax?

@alexreinking
Copy link
Member

alexreinking commented Mar 3, 2022 •
edited
Loading

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

steven-johnson commented Mar 3, 2022

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
@AtilaSaraiva AtilaSaraiva mentioned this pull request Jul 3, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexreinking alexreinking alexreinking approved these changes

@abadams abadams Awaiting requested review from abadams

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@steven-johnson @abadams @alexreinking