FaceCache and FaceIterator

[KnutAM]

Ref #468
Edit after discussion with @fredrikekre :

• Complete after Rework CellIterator by extracting out the caching part #546
• Rename to FaceIterator and introduce FaceCache (following the current definition of Faces)
• Describe both methods in docs

[termi-official]

Some questions on this.

1. Can we uniquely iterate over all faces?
2. What is a face for the new iterator in the case of embedded elements?
3. Does this work for elements with mixed faces (e.g. prism elements)?

[codecov-commenter]

Codecov Report

Patch coverage: 87.17% and project coverage change: +1.11 🎉

Comparison is base (8457cd0) 91.63% compared to head (258eaec) 92.74%.

❗ Current head 258eaec differs from pull request most recent head b0e2400. Consider uploading reports for the commit b0e2400 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #495      +/-   ##
==========================================
+ Coverage   91.63%   92.74%   +1.11%     
==========================================
  Files          32       28       -4     
  Lines        4650     4232     -418     
==========================================
- Hits         4261     3925     -336     
+ Misses        389      307      -82     

Impacted Files	Coverage Δ
src/iterators.jl	94.05% <87.17%> (+9.68%)	⬆️

... and 27 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[KnutAM]

This iterator iterates over a given Set{FaceIndex} and provides the convenience associated with the CellIterator.
Specifically, it iterates over a FaceCache that updates the cell's faceid and caches the return-values of celldofs, cellcoords, and cellnodes. It can also conveniently reinit! facevalues.

1 Can we uniquely iterate over all faces?

Yes, wrt. each FaceIndex in the grid, no wrt. each unique face (as in a face being considered the same item when shared between cells). The latter would require features that to my knowledge are not currently available in Ferrite, but could be realized by adding another FaceIndex-type, that contain the cell and face ids for both the participating cells.

2 What is a face for the new iterator in the case of embedded elements?

I think this just follows from what a FaceIndex is defined as for embedded elements. (I haven't thought too much about that though)

3 Does this work for elements with mixed faces (e.g. prism elements)?

I think the iterator would work fine, but I think we would have a problem with the facevalues then,
because you would need different facevalues for different faces. Consequently, it would be up to the user to ensure that
the given Set{FaceIndex} is only over faces that are compatible with the face-value you would like to reinit!
Currently, the iterator only checks for the same cells in the case of MixedDofHandler (same as for CellIterator)
But a check could be added if we have such elements (do we?)

[termi-official]

Thanks for the quick reply Knut!

[...] adding another FaceIndex-type, that contain the cell and face ids for both the participating cells.

As a note here, this approach will just work if we have conforming elements with the constraint that faces match 1:1.

I think this just follows from what a FaceIndex is defined as for embedded elements.

I also think so. I think we should still handle this case in the tests, either by checking that this errors out of it it handles the "faces of embedded elements" correctly.

But a check could be added if we have such elements (do we?)

I tried to implement such elements several times and always hit a dead end. I think it makes sense to merge the dof handlers first and then introduce such elements. Still, we should make the error message clear for future reference.

[termi-official]

Maybe this is also a good PR in which we can explore possibilities to define actual faces.

[KnutAM]

Maybe this is also a good PR in which we can explore possibilities to define actual faces.

To have the possibility of defining a face as a separate entity, not connected to one specific cell for each face, would indeed be very nice. However, I would say that should be a separate PR, as that requires something completely different.

This PR basically makes the following code a bit easier (and a bit more general, including resizing thanks to #546)

c_coords = getcoordinates(grid, 1)
c_dofs = celldofs(dh, 1)
c_nodes = collect(getcells(grid, 1).nodes)
for face in getfaceset(grid, "right")
 getcoordinates!(c_coords, grid, face[1])
 celldofs!(c_dofs, dh, face[1])
 Ferrite.cellnodes!(c_nodes, grid, face[1])
 reinit!(facevalues, c_coords, face[2])
end

[KnutAM]

I think this just follows from what a FaceIndex is defined as for embedded elements.

I also think so. I think we should still handle this case in the tests, either by checking that this errors out of it it handles the "faces of embedded elements" correctly.

Do you have a specific case in mind? When you mean embedded elements, you mean elements of different geometric dimensionality right? In that case, the error will arise if trying to reinit! facevalues that are not matching the element, which in a way is unrelated to this iterator? I can try to add a test case for a mixed grid tomorrow...

But a check could be added if we have such elements (do we?)

I tried to implement such elements several times and always hit a dead end. I think it makes sense to merge the dof handlers first and then introduce such elements. Still, we should make the error message clear for future reference.

I'm not sure how to do that without introducing extra features, e.g. a trait for the celltype if the faces are mixed. Seems like that should be considered when such an element is added?

[termi-official]

Maybe this is also a good PR in which we can explore possibilities to define actual faces.

To have the possibility of defining a face as a separate entity, not connected to one specific cell for each face, would indeed be very nice. However, I would say that should be a separate PR, as that requires something completely different.

This PR basically makes the following code a bit easier (and a bit more general, including resizing thanks to #546)

c_coords = getcoordinates(grid, 1)
c_dofs = celldofs(dh, 1)
c_nodes = collect(getcells(grid, 1).nodes)
for face in getfaceset(grid, "right")
 getcoordinates!(c_coords, grid, face[1])
 celldofs!(c_dofs, dh, face[1])
 Ferrite.cellnodes!(c_nodes, grid, face[1])
 reinit!(facevalues, c_coords, face[2])
end

I understand the PR and like the design. I think it is really a big improvement over the design of the previous PR and closer to the how Ferrite is designed in general. I am just a bit concerned about adding features which may change drastically in the near future. But I agree, that defining faces is a related, but separate issue.

Do you have a specific case in mind? When you mean embedded elements, you mean elements of different geometric dimensionality right? In that case, the error will arise if trying to reinit! facevalues that are not matching the element, which in a way is unrelated to this iterator? I can try to add a test case for a mixed grid tomorrow...

Sorry, I forgot reinit fails already (which should not, because the mapping is well-defined).

[KnutAM]

Thanks for the feedback!

I understand the PR and like the design. I think it is really a big improvement over the design of the previous PR and closer to the how Ferrite is designed in general. I am just a bit concerned about adding features which may change drastically in the near future. But I agree, that defining faces is a related, but separate issue.

Yes, sorry, didn't mean to overexplain! That's a fair point, but I think that changing the definition of a Cell and what is implied with a face would be disruptive in any case but wouldn't necessarily affect this PR too much, perhaps in the naming.
One could denote what we are doing here as a CellFace instead of just Face, but then it should be updated to CellFaceIndex as well which is a change I think is better kept for doing the big overhaul of the geometric/topological interfaces.

Sorry, I forgot reinit fails already (which should not, because the mapping is well-defined).

OK, then I'll remove that todo for now and see what Fredrik says for the overall details first.