Add more copying to DistributedMesh and ReplicatedMesh copy constructors #2028
Conversation
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
|
The added copying of names looks like a win for correctness. 👍 Using skip_find_neighbors looks like a win for consistency and efficiency. 👍 We might also want to switch that map to unordered_map if this code path is about to get much heavier use? But before we merge this - can you explain to me how it's fixing #2026? The find_neighbors operation here ought to be redundant, but not incorrect! |
|
It looks to me like we are already using |
|
Ah, you're right, that got fixed a year and a half ago. Sorry, I think I was looking at "git log -p" code rather than the latest. Ok, let's merge this as-is, but definitely not close out #2026. We might be avoiding the bug but I doubt we'll be able to avoid it forever, and it would sure be great to fix it before a user hits it again! |
roystgnr
added a commit
to roystgnr/libmesh
that referenced
this pull request
Apr 6, 2019
We've never been correctly copying remote_elem neighbor links in the skip_find_neighbors code path (or rather, we've been copying them correctly and then accidentally *undoing* them), and since libMesh#2028 made skip_find_neighbors the default behavior in more code paths, that bug was starting to be triggered e.g. in the libMesh#1938 tests. This commit gets remote_elem into the old-to-new-elements map so the skip_find_neighbors code path will recognize it, and removes a bit of redundancy in the rest of the old-to-new-elements map initialization, and moves the manual remote_elem-specific copying code into the !skip_find_neighbors path to avoid redundancy with that.
roystgnr
added a commit
to roystgnr/libmesh
that referenced
this pull request
Apr 7, 2019
We've never been correctly copying remote_elem neighbor links in the skip_find_neighbors code path (or rather, we've been copying them correctly and then accidentally *undoing* them), and since libMesh#2028 made skip_find_neighbors the default behavior in more code paths, that bug was starting to be triggered e.g. in the libMesh#1938 tests. This commit gets remote_elem into the old-to-new-elements map so the skip_find_neighbors code path will recognize it, and removes a bit of redundancy in the rest of the old-to-new-elements map initialization, and moves the manual remote_elem-specific copying code into the !skip_find_neighbors path to avoid redundancy with that.
jwpeterson
pushed a commit
that referenced
this pull request
Apr 9, 2019
We've never been correctly copying remote_elem neighbor links in the skip_find_neighbors code path (or rather, we've been copying them correctly and then accidentally *undoing* them), and since #2028 made skip_find_neighbors the default behavior in more code paths, that bug was starting to be triggered e.g. in the #1938 tests. This commit gets remote_elem into the old-to-new-elements map so the skip_find_neighbors code path will recognize it, and removes a bit of redundancy in the rest of the old-to-new-elements map initialization, and moves the manual remote_elem-specific copying code into the !skip_find_neighbors path to avoid redundancy with that. Refs #2029.
lindsayad
added a commit
to lindsayad/libmesh
that referenced
this pull request
Apr 20, 2023
The lines I'm removing will iterate over all the boundary IDs in the boundary info and then add to the map from id to name after querying the boundary info we're copying from. However, methods like `get_sideset_name` will happily return an empty string if the query ID doesn't have a corresponding name. And then insert the empty string into the boundary info we're copy assigning. This pedantically makes the boundary info non-equal. I added these bad lines back in libMesh#2028. Roy added the correct fix in libMesh#2634. Let's just rely on that. Refs libMesh#3516
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The decision to
skip_find_neighborsin the call toUnstructuredMesh::copy_nodes_and_elementsis based partially on #2026 but I also think that it's more consistent with the spirit of a copy constructor...it's copying over the neighbour mapping of the other mesh.