Change tag/face mapping argument of _compute_facial_adjacency_from_vertices - #352
Open
majosm wants to merge 10 commits into
Open
Change tag/face mapping argument of _compute_facial_adjacency_from_vertices#352majosm wants to merge 10 commits into
_compute_facial_adjacency_from_vertices#352majosm wants to merge 10 commits into
Conversation
…rtices change the structure to something that can be constructed more efficiently
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Alex Fikl <alexfikl@gmail.com>
Co-authored-by: Alex Fikl <alexfikl@gmail.com>
majosmforce-pushed
the
optimize-tag-face-mapping
branch
from
October 12, 2022 22:12
8c3430a to
db96dcaComparemajosmforce-pushed
the
optimize-tag-face-mapping
branch
from
October 12, 2022 22:14
db96dca to
65f7f01Comparemajosm
commented
Oct 12, 2022
CollaboratorAuthor
On my laptop, for a mesh with 1.2M elements:
|
majosm
marked this pull request as ready for review
October 13, 2022 15:48
inducer
reviewed
Oct 17, 2022
inducer
left a comment
Owner
There was a problem hiding this comment.
Some comments from an initial scroll.
| faces=np.concatenate([ids.faces for ids in face_ids_list])) | ||
| def _find_matching_index_pairs_merged(indices): |
Owner
There was a problem hiding this comment.
Describe shape of indices (and describe the axis along which the return value indexes).
| return np.stack((order[match_indices], order[match_indices+1])) | ||
| def _find_matching_index_pairs(left_indices, right_indices): |
Owner
There was a problem hiding this comment.
Describe shape of left_indices and right_indices (and describe the axis along which indiex_pairs indexes).
Comment on lines
+1426
to
+1427
| :class:`numpy.ndarray` of shape ``(2, nfaces)`` containing | ||
| the element and face indices of each tagged face in the group. |
Owner
There was a problem hiding this comment.
I'm guessing you mean element indices in tag_to_group_faces[0] and face indices in tag_to_group_faces[1]? If so, say so.
| """ | ||
| Return an array of dimension ``(2, nmatches)`` containing pairs of indices into | ||
| *indices* representing entries that are the same. | ||
| """ |
Owner
There was a problem hiding this comment.
Document the interface consequences of lexsort being stable that are being used below.
| """ | ||
| index_pairs = _find_matching_index_pairs_merged( | ||
| np.concatenate((left_indices, right_indices), axis=1)) | ||
| index_pairs[1, :] -= left_indices.shape[1] |
Owner
There was a problem hiding this comment.
Might it be worthwhile to assert that these are in range (i.e. not drawn into negative)?
| face_index_pairs = _find_matching_index_pairs( | ||
| tagged_elements_and_faces.T, | ||
| np.stack((bdry_elements, bdry_element_faces))) | ||
| face_indices = face_index_pairs[1, :] |
Owner
There was a problem hiding this comment.
Suggested change
| face_indices=face_index_pairs[1, :] | |
| bdry_face_indices=face_index_pairs[1, :] |
| is_tagged = np.full(len(bdry_elements), False) | ||
| for tag, tagged_elements_and_faces in tag_to_group_faces[igrp].items(): | ||
| face_index_pairs = _find_matching_index_pairs( |
Owner
There was a problem hiding this comment.
Suggested change
| face_index_pairs=_find_matching_index_pairs( | |
| vol_bdry_index_pairs=_find_matching_index_pairs( |
inducer
reviewed
Oct 17, 2022
| Return an array containing pairs of indices into *indices* representing entries | ||
| that are the same. | ||
| Given an array *indices* of shape ``(N, nindices)`` containing integer-valued |
Owner
There was a problem hiding this comment.
Suggested change
| Givenanarray*indices*ofshape``(N, nindices)``containinginteger-valued | |
| Givenanarray*indices*ofshape``(N, ntuples)``containinginteger-valued |
Comment on lines
+1358
to
+1359
| Returned matches are ordered such that the second element of the pair occurs | ||
| after the first in *indices*. |
Owner
There was a problem hiding this comment.
assert (result[0] < result[1]).all()?
(Also improve phrasing here.)
| # Indices into left_indices are the same as indices into all_indices, but need | ||
| # a conversion for right_indices | ||
| index_pairs[1, :] -= left_indices.shape[1] | ||
| assert index_pairs[1, :] >= 0 # Sanity check |
Owner
There was a problem hiding this comment.
Suggested change
| assertindex_pairs[1, :] >=0# Sanity check | |
| assert(index_pairs[1, :] >=0).all()# Sanity check |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Now accepts a list of dicts (one per mesh group), where each dict maps a tag to a numpy array of element/face indices that belong to that tag. This removes the need to use Python loops over elements when generating and using the tag/face mappings.