-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the property storage of XFEM deleted element and side #17067
Conversation
Job Documentation on 45339a1 wanted to post the following: View the site here This comment will be updated on new commits. |
69a4d03
to
f3e7700
Compare
@rwcarlsen could you review this PR? Thanks. |
f3e7700
to
b2732ee
Compare
|
||
// remove the property storage of deleted element/side | ||
(*_material_data)[0]->eraseProperty(elem_to_delete); | ||
(*_bnd_material_data)[0]->eraseProperty(elem_to_delete); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there are several places in the XFEM class/file that you delete elements. And you remove boundary info, erase material properties, etc. along with the actual element deletions. Would it be better to have a special removeElem function that safely did all these things atomically? - That way you won't have to worry about forgetting to do any of these things whenever you are deleting elements. Or are the situations for element deletion different enough that they all need different subsets of things done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct that there are other three places that call element deletion. However those deletion is for elements on the displaced mesh, and there is no need to remove their material properties because undisplaced and displaced mesh share same material properties.
@@ -255,13 +255,13 @@ class MaterialPropertyStorage | |||
return _prop_names.count(retrievePropertyId(prop_name)) > 0; | |||
} | |||
|
|||
/// Remove the property storage and element pointer from internal data structures | |||
void eraseProperty(const Elem * elem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a sentence to the doc comment saying "Use this when elements are deleted so we don't end up with invalid elem pointers (for e.g. stateful properties) hanging around in our data structures." - or something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add what you suggest. Thanks.
/** | ||
* Remove the property storage and element pointer from MaterialPropertyStorage data structures | ||
*/ | ||
void eraseProperty(const Elem * elem) { _storage.eraseProperty(elem); }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a sentence to the doc comment saying "Use this when elements are deleted so we don't end up with invalid elem pointers (for e.g. stateful properties) hanging around in our data structures." - or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add what you suggest. Thanks.
b2732ee
to
45339a1
Compare
Job Modules Parallel on 45339a1 : invalidated by @jiangwen84 failure is not related to this PR |
@rwcarlsen Not sure if you remember this -- we actually depend on this "manual" erasing of material properties to copy some material properties from an old element to a new element. If the material property storage is actually cleaned up on time, that'll break something in xfem. |
@rwcarlsen The related PR I was talking about is #16278 . |
closes #17063