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

Improve Topology construction and fix MPI/NBX bug #3512

Merged
merged 36 commits into from
Nov 15, 2024

Conversation

garth-wells
Copy link
Member

@garth-wells garth-wells commented Nov 15, 2024

This change gives a mesh Topology a 'fully formed' constructor, i.e. a Topology object is complete from construction in that it has vertices and cells. Other entries are derived from these. This follows the DOLFINx design philosophy where we avoid creating half-baked objects with unclear state.

These changes triggered are seemingly unrelated bug in the NBX algorithm related to MPI. Consecutive, fast calls to the NBX could result in corrupted data since the non-blocking MPI calls used the same tag. The tag passed to MPI functions is now an optional argument.

Change also remove duplicated code in Topology constructors.

Resolves some of the point in #3506.

@garth-wells garth-wells added the enhancement New feature or request label Nov 15, 2024
@garth-wells garth-wells added this to the 0.10.0 milestone Nov 15, 2024
@jorgensd
Copy link
Member

I would suggest refactoring the set_connectivity function as well, as well as set_index_map, as explained in #3506

@garth-wells
Copy link
Member Author

I would suggest refactoring the set_connectivity function as well, as well as set_index_map, as explained in #3506

I think further changes should come later because they are much more intrusive for the interface, and some points need careful thought. This changes doesn't really affect the API (since users do not generally use the Topology constructor), and the PR fixes a bug.

@jorgensd
Copy link
Member

I would suggest refactoring the set_connectivity function as well, as well as set_index_map, as explained in #3506

I think further changes should come later because they are much more intrusive for the interface, and some points need careful thought. This changes doesn't really affect the API (since users do not generally use the Topology constructor), and the PR fixes a bug.

What I am suggesting isn't very intrusive to the API as most users do not:

  1. Attach index maps to the topology themself (as this is done through create_entities)
  2. Does not clear the entities post creation.
  3. Does set connectivity manually (as it is done through create connectivity).

The clear options would be a simple change for anyone using the current interface explicitly.

@chrisrichardson
Copy link
Contributor

I think there is already enough in this PR.

@garth-wells garth-wells added this pull request to the merge queue Nov 15, 2024
Merged via the queue into main with commit 5323846 Nov 15, 2024
27 checks passed
@garth-wells garth-wells deleted the garth/topology-build branch November 15, 2024 13:42
jorgensd added a commit to jorgensd/adios4dolfinx that referenced this pull request Nov 17, 2024
jorgensd added a commit to jorgensd/adios4dolfinx that referenced this pull request Nov 17, 2024
* Changes due to introduction of consensus tag introduced in FEniCS/dolfinx#3512

* Bump install action and version number
schnellerhase pushed a commit to schnellerhase/dolfinx that referenced this pull request Dec 28, 2024
* Work on Topology constructor

* Fix compilation

* Update

* Fixes

* Sub-topology fix

* Wrapper update

* Doc fixes

* Continue on error

* Updates

* Doc fix

* Debug

* Debug

* Doc fix

* Update demo

* Add barrier

* Use PCX

* Increase problem size

* Parameterise demo

* Small edits

* Pass tag to nbx function

* Test update

* Tidy up

* Test update

* Simplify

* Simplification

* Tidy

* Simplifications

* Undo changes

* Simplify

* Tidy

* Wrapper simplification

* Update doc

* Remove stray line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants