Make ordering conventions explicit #13
Conversation
The Face type should be good to delete once these changes are applied
|
Awesome, that looks really nice 💯 I will try to test locally today, meanwhile could you please rename the folder Thanks for the awesome improvements. |
There was a problem hiding this comment.
Hi @serenity4 , this PR isn't very easy to review. It is doing multiple things at the same time, and this is hard to keep track in the future. Could you maybe undo the deletion of the Face type, and just handle the ordering convention concept here? There is a lot of file movement to different folders, and a lot of s/Face/Polytope/ that is not related to ordering conventions.
We need a separate PR later to consider the removal of Face.
|
|
||
| ## Example | ||
| ``` | ||
| Δ = connect((1,2,3), Triangle) | ||
| ``` | ||
| """ | ||
| connect(list::NTuple{N,Int}, F::Type{<:Face}) where {N} = Connectivity{F,N}(list) | ||
| connect(list::NTuple{N,Int}, F::Type{<:Polytope}) where {N} = Connectivity{F,N}(list) |
There was a problem hiding this comment.
Although I like all your suggestions so far, it would be nice if you could draft PRs more carefully. I am spotting a lot occurrences of incomplete changes like this where you chage Face to Polytope and forget to update the type parameter from F to P.
| @@ -84,6 +84,10 @@ export | |||
| Connectivity, | |||
| facetype, connect, materialize, | |||
|
|
|||
| # ordering conventions | |||
| Ordering, connectivity, | |||
There was a problem hiding this comment.
You are exchanging two terms in this PR: ordering and convention. Can we agree on a single term "OrderingConvention" and use it consistently in the PR?
| """ | ||
| Connectivity function provided by an `Ordering` subtype used to construct k-faces of a polytope from vertices (0-faces). | ||
| """ | ||
| function connectivity end |
There was a problem hiding this comment.
Please configure your IDE to not remove the last EOL. VS Code does that by default unfortunately.
|
|
||
| connectivity(::Type{<:Tetrahedron{GMSH}}, ::Type{GMSH}, ::Val{2}) = | ||
| connect.([(1,2,3),(1,4,3),(1,4,2),(2,3,4)], Triangle) | ||
|
|
There was a problem hiding this comment.
Sometimes you add 2 blank lines, sometimes you add 1 blank line between function definitions. I know this may sound picky, but it is important in the long run to be pedantic about these things.
|
In summary, it would be nice if you could
|
|
I've addressed item (1) in #14. Requesting your review over there 👍 |
|
Appreciate the feedback. Tbh I am not used to be as clear when making commits/PRs, particularly for having a good history, but that's better practice indeed. I'll try to be as careful as I can for future PRs. Don't hesitate to continue such feedback when needed. |
|
Now that #14 has been merged, we can either refactor this PR or start a clean one from a new branch out of master. It would be nice to generalize the ordering convention as you proposed here. Please let me know how you prefer to proceed. |
|
I think I'll just restart anew, it will be easier. |
This PR implements ordering conventions, that solves #11.
The old
Facetype is still there, but not used anymore, so it can be safely deleted (in this PR or another).As discussed in #12, equality of polytopes imply equality of all k-faces. Therefore, I modified equality tests between polytopes so that only vertices are tested for equality. It only affects tests for
UnstructuredMeshes. The tested behavior is exactly the same as before this PR.