Separate face matching from _compute_facial_adjacency_from_vertices#207
Conversation
majosm
force-pushed
the
separate-face-matching
branch
from
ff040e6 to
245149d
Compare
inducer
left a comment
inducer
left a comment
There was a problem hiding this comment.
Thanks! Some review bits that came up during our meeting.
|
Unsubscribing... @-mention or request review once it's ready for a look or needs attention. |
majosm
force-pushed
the
separate-face-matching
branch
5 times, most recently
from
b5820a6 to
a156f8f
Compare
majosm
force-pushed
the
separate-face-matching
branch
from
a156f8f to
0e7dbe6
Compare
|
Ready for a look @inducer. I temporarily restored |
inducer
left a comment
inducer
left a comment
There was a problem hiding this comment.
Thanks! This looks great functionally. Just some nitpicking around docs and explanation, so that (hopefully) reading this becomes easier, then this is ready to go.
|
|
||
| def _match_faces_by_vertices(groups, face_ids, vertex_index_map_func=None): | ||
| """ | ||
| Return matching faces in *face_ids*, where two faces match if they have the |
There was a problem hiding this comment.
Return matching faces
In what form?
| indices into *face_ids*. *vertex_index_map_func* is used to map vertices to | ||
| other vertices; it must accept (possibly multidimensional) numpy arrays of | ||
| vertex indices. |
There was a problem hiding this comment.
Document arguments. Specifically, document the signature and return type (also acceptable shapes) of vertex_index_map_func.
Also document the return type and meaning of returned data from _match_faces_by_vertices.
Don't see that anywhere? What's this referring to? |
Co-authored-by: Andreas Klöckner <inform@tiker.net>
Co-authored-by: Andreas Klöckner <inform@tiker.net>
Co-authored-by: Andreas Klöckner <inform@tiker.net>
majosm
force-pushed
the
separate-face-matching
branch
from
3dde10c to
043ea11
Compare
It's right above |
majosm
force-pushed
the
separate-face-matching
branch
from
043ea11 to
a47023f
Compare
inducer
left a comment
inducer
left a comment
There was a problem hiding this comment.
Thanks! This LGTM now. I'll wait for you to remove _FlatFacialAdjacencyData. Let me know if/when it's ready to merge from your end.
This reverts commit fe50f62.
|
Good to go @inducer. |
|
Thanks! (Hope you're OK with me squashing this... if not, please tell me, although I won't be able to do much to fix it.) |
2 participants
Pulled out of #204 to declutter.
_FlatFacialAdjacencyDataagain before merging.