Skip to content

Improve python exposure of determine_point_ownership #3344

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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

ordinary-slim
Copy link
Contributor

Python wrapper around nanobind exposure of determine_point_ownership and extra optional arguments cells so that the search is performed only for cells.

Copy link
Member

@jorgensd jorgensd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Could you add a test for this?:)

@ordinary-slim
Copy link
Contributor Author

Looks good to me. Could you add a test for this?:)

I added a test comparing bounding box tree results to determine_point_ownership. Another possibility would be to look at post files and hardcode rank-cell data into a test.

Comment on lines 516 to 520
def test_po_against_bb_tree(mesh,points,cells):
cells = np.array([cell for cell in cells if cell < cell_map.size_local])
num_points = points.shape[0]
po = determine_point_ownership(mesh,points,cells)
tree = bb_tree(mesh, mesh.topology.dim, cells)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run ruff format and ruff check --fix on the Python files to assure appropriate formatting.

@ordinary-slim
Copy link
Contributor Author

ordinary-slim commented Aug 21, 2024

Sorry, I made a mess with my branches in my fork and closed the pull request by mistake. Is it possible to reopen it pointing to https://github.com/ordinary-slim/dolfinx/tree/main ?

@jorgensd @jhale

@jorgensd
Copy link
Member

Sorry, I made a mess with my branches in my fork and closed the pull request by mistake. Is it possible to reopen it pointing to https://github.com/ordinary-slim/dolfinx/tree/main ?

@jorgensd @jhale

I think you have to reopen it from your repository. See: https://gist.github.com/Farhan-Haseeb/fd4cbb476de4e2931c322d7bacb75e5f

@ordinary-slim
Copy link
Contributor Author

Sorry, I made a mess with my branches in my fork and closed the pull request by mistake. Is it possible to reopen it pointing to https://github.com/ordinary-slim/dolfinx/tree/main ?
@jorgensd @jhale

I think you have to reopen it from your repository. See: https://gist.github.com/Farhan-Haseeb/fd4cbb476de4e2931c322d7bacb75e5f

I can't see the PR from my repository. I deleted the original branch of the PR and it automatically closed it. The reopen button is greyed out:
Screenshot

The link you provided says:

If you changed the branch name or deleted the branch, the reopen pull request button will be disabled so first you need to click the restore branch button in the right section of your commit ( change branch name / delete branch commit) inside your pull request then the reopen button will be enabled

But I can't find this restore branch button. Could you look for it on your side? Else I think I have to open a new PR. Sorry for the inconvenience!

@jorgensd
Copy link
Member

I cannot reopen. Please open a new PR:)

@ordinary-slim
Copy link
Contributor Author

ordinary-slim commented Aug 22, 2024

Ok, I had to recover the deleted branch with the same name then I could reopen, as per this stackoverflow comment. Sorry for the inconvenience!

@ordinary-slim ordinary-slim reopened this Aug 22, 2024
h = dtype(1.0 / num_cells_side)
if tdim == 2:
mesh = create_unit_square(
MPI.COMM_WORLD, num_cells_side, num_cells_side, CellType.quadrilateral, dtype=dtype
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be. parameterised across the mesh? (triangle, quad, tet, hex)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as the test is now it needs the elements to coincide with their bounding boxes. I'll commit a version that covers triangles and tet elements.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great, thank you.

A large padding value will increase the run-time of the code by orders
of magnitude. General advice is to use a padding on the scale of the
cell size.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove new line?

/// where src_owner is a list of ranks corresponding to the input
/// points. dest_owner is a list of ranks corresponding to dest_points,
/// the points that this process owns. dest_cells contains the
/// @return Point ownership data with fields `src_owner`, `dest_owner`, `dest_points`, `dest_cells`,
Copy link
Member

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.

@ordinary-slim
Copy link
Contributor Author

FYI: The geometry test test_compute_closest_sub_entity fails at 5 and 8 processors. It doesn't fail at 1,2,3,4,6 nor 7 processors. On the latest commit of the main branch.

`test_determine_point_ownership` tests for triangle and tetra
  elements.
@ordinary-slim
Copy link
Contributor Author

Let me know if I should move the last two commits (non-defaulting padding argument in BoundingBoxTree constructors) to a new PR.

@ordinary-slim
Copy link
Contributor Author

Bumping this @jhale

@mscroggs mscroggs modified the milestones: 0.9.0, 0.10.0 Nov 11, 2024
@jhale
Copy link
Member

jhale commented Nov 29, 2024

I'd like to get this merged - can you address the two final small comments?

Copy link
Member

@garth-wells garth-wells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be sure to run clang-format on C++ code and use ruff to format Python code.

- Switched c++ optional argument to std::optional
- Formated files modified by PR
@ordinary-slim
Copy link
Contributor Author

I'd like to get this merged - can you address the two final small comments?

Ok! I'll try to reply fast.

@jhale
Copy link
Member

jhale commented Jul 21, 2025

I will try to update and merge this, looks pretty solid.

@ordinary-slim
Copy link
Contributor Author

I will try to update and merge this, looks pretty solid.

Thanks!

I am under the impression that the test is too convoluted / large hence the slow progress on this PR. Let me know if you have an idea for a different test. On the other hand, I believe it is a good test in the sense that it computes the desired output in a different fashion and compares it to the actual output of the function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants