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

Make topology const in locate_dofs_topological #2885

Merged
merged 7 commits into from
Nov 12, 2023
Merged

Conversation

IgorBaratta
Copy link
Member

@IgorBaratta IgorBaratta commented Nov 10, 2023

As for other functions in dolfinx, the user should call create_connectivity explicitly.
Locating dofs shouldn't modify the topology.

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.

I've got mixed emotions regarding this. If one wants to call locate_dofs_topological, this is clearly needed, and it is not clear why we want to force users to call it for this function.

Also note that if we remove it, we need proper error handling (a simple assert isn't enough), as this is not checked in release mode. Then we would need a runtime error saying "Missing connectivity between entities of dim {dim} and {tdim}".

@IgorBaratta
Copy link
Member Author

IgorBaratta commented Nov 10, 2023

I think you're right about the error handling.
But other functions already require the user to call create_connectivity and create_entities, for example exterior_facet_indices.

@jorgensd
Copy link
Member

I think you're right about the error handling. But other functions already require the user to call create_connectivity and create_entities, for example exterior_facet_indices.

I know, and if we want to be consistent, we follow your code (and add an appropriate error message).
I just feel like we should add to the doc-strings what connectives we expect to ensure that it is easily available information, and that people don't have to run the code and get an error message to know what they need.

@francesco-ballarin
Copy link
Member

I for sure agree with all comments above, but I just want to point that these commits did not change a single test or demo. It seems to me that, for all intents and purposes, in any of our actual use cases (C++ demos, C++ tests, python demos, python tests) those entities and connectives are indeed already computed in practice, so most of the users won't even notice this change.

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.

This is good, but need to change assertions to exceptions.

On Python side we're ok with more implicit behaviour so any required initialisation can happen in the Python layer.

cpp/dolfinx/fem/DirichletBC.cpp Outdated Show resolved Hide resolved
@garth-wells garth-wells added this pull request to the merge queue Nov 12, 2023
Merged via the queue into main with commit 4af1853 Nov 12, 2023
20 checks passed
@garth-wells garth-wells deleted the igor/topology_const branch November 12, 2023 16:42
jorgensd added a commit to Wells-Group/TEAM30 that referenced this pull request Nov 16, 2023
jorgensd added a commit to Wells-Group/TEAM30 that referenced this pull request Nov 20, 2023
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.

4 participants