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 7 commits
9f2597a
434aa5e
5855796
881434c
5b6726f
b06ab5a
aa7d47e
06c9928
7be7013
75a3bc5
05c609a
5baf1c2
6fda65d
5e1246d
9420238
74b9a22
a969271
3e481f0
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.
I think this documentation should be moved out to the struct itself.
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.
This argument could be converted to
std::optional
- can be done on later patch.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.
As this argument is critical to the behaviour of the function it is best to leave it as a non-default. Then user's need to engage with it, rather than not realise it exists.
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.
padding
is forwarded toBoundingBoxTree
constructor where it is also default.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.
@ordinary-slim Please make it non-default argument in both bounding box tree and here.
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.
As this argument is critical to the behaviour of the function it is best to leave it as a non-default. Then user's need to engage with it, rather than not realise it exists.
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.
I am bit reluctant to do this change since if
cells
is left as default then the argument ordermesh
,points
,padding
,cells
feels a bit awkward to me. Moreoverbb_tree
also has a default value for padding.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.
@jorgensd @garth-wells What do you think about this
padding
argument? My view is that as it's critical to the operation of the bounding box and we cannot provide a sensible default, it's better not to have it hidden as a default parameter.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.
I think it is a crucial parameter. We have already rewritten the API for non-matching interpolation since 0.7, so I think it is fine to make it required.
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.
@ordinary-slim Please make it non-default argument in both bounding box tree and here.
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.
Are the fields of PointOwnershipData documented in the Python wrapped class?
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.
dolfinx/python/dolfinx/geometry.py
Lines 47 to 61 in 6fda65d
The doc is the same as on the c++ side.
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.
Looks good, thanks for pointing it out to me.
The C++ documentation for the return argument describing PointOwnershipData looks over-specified given the struct is now properly documented.
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.
Removed most of it.
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.
Remove new line?