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

Favor exterior_facet_indices over locate_entities_boundary for retrieving complete boundary #3283

Merged

Conversation

schnellerhase
Copy link
Contributor

This allows for the usage of locate_entities_boundary without providing a 'all true' marker to identify all boundary facets. This increases readability of the use cases, which before had to manually encode this all true function, done in different ways, and refrains from evaluating any marker in these cases altogether.

Fixes schnellerhase#16

@jorgensd
Copy link
Sponsor Member

We already have dolfinx.mesh.exterior_facet_indices that covers this case (https://docs.fenicsproject.org/dolfinx/v0.8.0/cpp/mesh.html#_CPPv4N7dolfinx4mesh22exterior_facet_indicesERK8Topology)
Is there a reason for adding a specialisation for the locate_entities_boundary function?

@schnellerhase
Copy link
Contributor Author

schnellerhase commented Jun 25, 2024

Right, that is also the one used in locate_entities_boundary before - and thus we could return there already.

Wasn't really aware of that functionality to be honest - but passing that through the same interface seems like a good way to go nevertheless. So the difference between a specially marked boundary and the default whole boundary is just the marker not a different function.

@jorgensd
Copy link
Sponsor Member

Right, that is also the one used in locate_entities_boundary before - and thus we could return there already.

Wasn't really aware of that functionality to be honest - but passing that through the same interface seems like a good way to go nevertheless. So the difference between a specially marked boundary and the default whole boundary is just the marker not a different function.

I am not sure it is worth adding special case handling inside an existing function when the original function is already exposed (and used) by people.

@schnellerhase
Copy link
Contributor Author

I see, maybe we switch to the explicit usage of exterior_facet_indices then in those instances of the here changed code? If they had been used in these demos, I probably would also have not thought about extending the interface in any way.

@jorgensd
Copy link
Sponsor Member

I see, maybe we switch to the explicit usage of exterior_facet_indices then in those instances of the here changed code? If they had been used in these demos, I probably would also have not thought about extending the interface in any way.

I would be happy with that, we should keep one demo to illustrate how to use numpy binary operators, but the rest can go I guess

@schnellerhase
Copy link
Contributor Author

schnellerhase commented Jun 25, 2024

On a second thought the exterior_facet_indices only allows access to the top.dim - 1 entities on the boundary, not entities of general dimension - which the special case handling allows for. But I assume this is rather nitpicking to rectify my changes.

@jorgensd
Copy link
Sponsor Member

On a second thought the exterior_facet_indices only allows access to the top.dim - 1 entities on the boundary, not entities of general dimension - which the special case handling allows for. But I assume this is rather nitpicking to rectify my changes.

I am happy to hear what others think.

However, one can use exterior_facet indices + compute_incident_entities to get all entities of lower dimension associated with those facets.

@nate-sime
Copy link
Contributor

I'm not really keen on the optional argument of entities provided to locate_entities_boundary. The philosophy of DOLFINx is to be completely explicit.

@schnellerhase
Copy link
Contributor Author

So the optional is of the table, I changed to the usage of exterior_facet_indices where possible. This makes an explicit computation of the connectivity necessary before hand.

@schnellerhase schnellerhase changed the title locate_entities_boundary default marker Favor exterior_facet_indices over locate_entities_boundary for retrieving complete boundary Jul 1, 2024
schnellerhase and others added 20 commits July 1, 2024 16:06
* Create mesh independent form compilator for Python interface

* Ruff formatting

* Mypy

* Parameterize test over all dtypes

* Ruff format

* Fix compile options handling

* Fix coeff and constant name mapping

* Ruff

* Remove extra definition of basix element and check that mesh c_el is consistent

* Add support for linear and bilinear forms

* Fix typo

* Apply suggestions from code review

Co-authored-by: Chris Richardson <[email protected]>

* Apply suggestions from code review

Co-authored-by: Chris Richardson <[email protected]>

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Igor Baratta <[email protected]>

* Apply suggestions from code review

* Add documentation and default scalar type

---------

Co-authored-by: Chris Richardson <[email protected]>
Co-authored-by: Igor Baratta <[email protected]>
Bumps [docker/build-push-action](https://github.com/docker/build-push-action) from 5 to 6.
- [Release notes](https://github.com/docker/build-push-action/releases)
- [Commits](docker/build-push-action@v5...v6)

---
updated-dependencies:
- dependency-name: docker/build-push-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Remove temporary workarounds

* Use dict comprehension
@schnellerhase schnellerhase force-pushed the default_locate_entites_boundary branch from 9be974d to e384854 Compare July 1, 2024 14:06
@schnellerhase
Copy link
Contributor Author

Sorry for the force push, had to change the author of the commits, git was setup wrong locally.

@garth-wells garth-wells added this pull request to the merge queue Jul 2, 2024
Merged via the queue into FEniCS:main with commit 02e9a4d Jul 2, 2024
16 checks passed
@schnellerhase schnellerhase deleted the default_locate_entites_boundary branch July 2, 2024 10:42
schnellerhase added a commit to schnellerhase/dolfinx that referenced this pull request Sep 11, 2024
…trieving complete boundary (FEniCS#3283)

* Introduce optionally empty marker for boundary entities

* Adapt to optional boundary marker

* Make use of simplifications in python code

* Make use of simplifications in cpp code

* Add xfail for now (FEniCS#3284)

* Data independent form compilation for Python interface (FEniCS#3263)

* Create mesh independent form compilator for Python interface

* Ruff formatting

* Mypy

* Parameterize test over all dtypes

* Ruff format

* Fix compile options handling

* Fix coeff and constant name mapping

* Ruff

* Remove extra definition of basix element and check that mesh c_el is consistent

* Add support for linear and bilinear forms

* Fix typo

* Apply suggestions from code review

Co-authored-by: Chris Richardson <[email protected]>

* Apply suggestions from code review

Co-authored-by: Chris Richardson <[email protected]>

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Igor Baratta <[email protected]>

* Apply suggestions from code review

* Add documentation and default scalar type

---------

Co-authored-by: Chris Richardson <[email protected]>
Co-authored-by: Igor Baratta <[email protected]>

* Bump docker/build-push-action from 5 to 6 (FEniCS#3279)

Bumps [docker/build-push-action](https://github.com/docker/build-push-action) from 5 to 6.
- [Release notes](https://github.com/docker/build-push-action/releases)
- [Commits](docker/build-push-action@v5...v6)

---
updated-dependencies:
- dependency-name: docker/build-push-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Improve check for facet->cell connectivity (FEniCS#3281)

* Improve check

* Update utils.cpp

* Revert optional and use

* Ruff

* Typo

* another

* Fix biharmonic

* Fix python biharmonic

* Fix python demo hdg

* last?

* fix test fem pipeline

* fem pipeline the last?

* Remove workarounds following upstream updates for CI (FEniCS#3289)

* Remove temporary workarounds

* Use dict comprehension

* Export missing compile time constants to python (FEniCS#3285)

Co-authored-by: papel <[email protected]>

* Update python/demo/demo_biharmonic.py

Co-authored-by: Jørgen Schartum Dokken <[email protected]>

* Apply suggestions

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Chris Richardson <[email protected]>
Co-authored-by: Jørgen Schartum Dokken <[email protected]>
Co-authored-by: Igor Baratta <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: papel <[email protected]>
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.

Add default 'all' marker to locate_entites_boundary
6 participants