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

Fix input arguments to dolfinx.mesh.refine_plaza #3018

Merged
merged 3 commits into from
Jan 28, 2024

Conversation

francesco-ballarin
Copy link
Member

Reported at https://fenicsproject.discourse.group/t/input-for-mesh-refinement-with-refine-plaza/13426

It seems to me that the current python API is certainly wrong (the option argument is not used anywhere), but I can't understand how it is possible that tests on main are currently passing since:

  • a required input argument to the nanobind wrapped function is not being passed in from python, but nanobind is not complaining.
  • the only two tests that exercise refine_plaza were providing input arguments in the wrong order.

Opening this as draft because I have no experience with mesh refinement, so I'd need help from someone else if the fix is more involved than sorting out wrong order of input arguments.

@francesco-ballarin

This comment was marked as outdated.

@francesco-ballarin
Copy link
Member Author

Either I misinterpreted the role of the second input argument to refine_plaza (most likely, given my lack of experience in this field)

Indeed, I did misinterpret it ;) I was passing facets in, rather than edges.

I pushed the additional test and I am marking this as ready for review.

@francesco-ballarin francesco-ballarin marked this pull request as ready for review January 27, 2024 07:05
@francesco-ballarin francesco-ballarin changed the title Fix input arguments to dolfinx.mesh.refine_plaza Fix input arguments to dolfinx.mesh.refine_plaza Jan 27, 2024
@jorgensd
Copy link
Member

The issue with the old code was that it actually didn't send in edges (which is a kwarg), and the other kwargs weren't given by name, hence applying them to the wrong variable.

Redistribute became the option, and thus the tests passed.
The fix in the PR seems good.

@francesco-ballarin francesco-ballarin added this pull request to the merge queue Jan 28, 2024
Merged via the queue into main with commit 201cea9 Jan 28, 2024
19 checks passed
@francesco-ballarin francesco-ballarin deleted the francesco/plaza-maybe-fix branch January 28, 2024 14:43
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.

2 participants