topology fixes and docs clean up

[koehlerson]

fixes #518 and #453

[codecov-commenter]

Codecov Report

Base: 92.79% // Head: 93.06% // Increases project coverage by +0.26% 🎉

Coverage data is based on head (7b42664) compared to base (930887e).
Patch coverage: 98.24% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   92.79%   93.06%   +0.26%     
==========================================
  Files          28       28              
  Lines        4222     4271      +49     
==========================================
+ Hits         3918     3975      +57     
+ Misses        304      296       -8     

Impacted Files	Coverage Δ
src/Grid/grid.jl	91.09% <98.24%> (+3.71%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[termi-official]

Thanks for the quick fix! The edge and face neighborhood should still have the same issue.

[koehlerson]

Thanks for the quick fix! The edge and face neighborhood should still have the same issue.

Face neighborhood doesn't have this issue, since it should be 1:1 correspondence for conforming grids. Nonconforming grids cannot encode the face neighborhood as discussed in the previous PR. For edge neighborhood this is true, but it is sort of exotic feature which probably isn't needed that often, but when it someone needs it I can do a follow up PR

[koehlerson]

ok nvm, I added the include_self functionality for edges. However, the statement about the face neighborhood should still be true. I think at this point I can't do better than just extending the returned array by the index itself for faces

[termi-official]

Thanks for the quick fix! The edge and face neighborhood should still have the same issue.

Face neighborhood doesn't have this issue, since it should be 1:1 correspondence for conforming grids. Nonconforming grids cannot encode the face neighborhood as discussed in the previous PR. For edge neighborhood this is true, but it is sort of exotic feature which probably isn't needed that often, but when it someone needs it I can do a follow up PR

Indeed, I agree that edge neighborhoods are not that commonly needed.

I think I will open up a follow up PR extending the docs, because something is inconsistent in my head. Maybe I can find what it is with an example.

                   7-----8-----9
                   |  3  |  4  |
                   4-----5-----6
                   |  1  |  2  |
                   1-----2-----3

For vertex 5 the neighborhood in the exclusive topology is spanned by the global vertices (2,4,6,8) expressed in the element local vertex indices, correct?

Now with this PR include_self=true includes also 5 expressed in the element local vertex indices, right?

For the right face on element 1 the neighborhood is the left face in element two, right?

So just a part of the face "(2,5)". With include_self=true, we now get both halves of the face "(2,5)" in our set. However, this is a semantically different neighborhood. Do you see what I mean? For the vertex we get for one the calls either the vertices connected by the 1-faces or the vertices setminus the vertex for which we query the neighborhood, while for the faces we obtain the opposing local portion or both local portions.

Do you agree that it would be better to rename the method to something different (i think they actually return something like to a vertex-patch, when I think about cell patches) and make the getneighborhood method consistent with the face neighborhood or vice versa?

[termi-official]

I found a few minor issues.

Also, I think test coverage for getneighborhood with FaceIndex is missing in 2d and 3d.

[koehlerson]

I found a few minor issues.

Also, I think test coverage for getneighborhood with FaceIndex is missing in 2d and 3d.

Since the FaceIndex getneighborhood dispatch does not reconstruct but instead only accesses the neighborhood matrix, I'd say that test coverage is missing is a bit stretched. There are tests which check the matrix entries and thus, the logic of the function. I can change them to use the function instead, but it's not really a benefit

[termi-official]

Since the FaceIndex getneighborhood dispatch does not reconstruct but instead only accesses the neighborhood matrix, I'd say that test coverage is missing is a bit stretched. There are tests which check the matrix entries and thus, the logic of the function. I can change them to use the function instead, but it's not really a benefit

These are two separate things. With one test we check if the data structure is correctly constructed. The next test is to check if the interface to the data structure works correctly. Note that we can make changes to the backend without changing the contract of the interface.

[termi-official]

LGTM. I think this can be merged @fredrikekre .

[fredrikekre]

You two are in charge of this code, so if you are happy with it I am happy with it :)