Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve python exposure of
determine_point_ownership
#3344base: main
Are you sure you want to change the base?
Improve python exposure of
determine_point_ownership
#3344Changes from 15 commits
9f2597a
434aa5e
5855796
881434c
5b6726f
b06ab5a
aa7d47e
06c9928
7be7013
75a3bc5
05c609a
5baf1c2
6fda65d
5e1246d
9420238
74b9a22
a969271
3e481f0
ec3f179
5313809
de68fc9
930f7af
08bf6e4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
std::optional
rather than an empty span.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhale I made
cells
a default argument at the c++ level (not at the wrapper level).Ideally this
local_cells
variable would be initialized inside of theempty
conditional but I could not figure it out since the span that wraps it dangles if the underlying vector goes out of scope. I tried using thestatic
keyword but didn't work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a nonissue, minor detail about code elegance maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting needs fixing - second and subsequent lines of docstring for an argument must be indented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you switch the C++ interface to
std::optional
, thisif
could be removed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
std::optional
forcells
.