-
Notifications
You must be signed in to change notification settings - Fork 300
[WIP] Debugging check that variables of discontinuous family live on just one elem->dim() #4345
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
[WIP] Debugging check that variables of discontinuous family live on just one elem->dim() #4345
Conversation
roystgnr
left a comment
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.
Is there a reason we want to cache this at the MeshBase level, rather than computing it in DofMap where we use it?
I ask partly just because I'm cringing at how this is certain to conflict with #3759 as-is, but partly because if we do want to cache this then I want us to get it right in operator=, locally_equals, etc, and that's going to be a pain.
5e9d3b1 to
315f6eb
Compare
|
will wait on libMesh/TIMPI#156 |
I've now moved it to |
| { | ||
| unsigned char elem_dims = 0; | ||
| for (const auto * const elem : mesh.active_subdomain_set_element_ptr_range(subdomains)) | ||
| elem_dims |= static_cast<unsigned char>(1u << cast_int<unsigned char>(elem->dim())); |
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.
is this chatgpt or does this make sense to you?
like the << and |= on chars
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.
well copilot explained it to me! Pretty creative
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 believe this is the sort of thing that's more clearly written with std::bitset in C++ code? But this was the standard idiom for bit sets in C code; I'm old enough that until I saw your comment it didn't even occur to me that the code here might be confusing.
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 really like it for being extremely lightweight compared to a std::set
src/base/dof_map.C
Outdated
| !more_than_one_bit_set, | ||
| "Discontinuous finite element families are typically associated with discontinuous Galerkin " | ||
| "methods. If degrees of freedom are associated with different values of element dimension, " | ||
| "then generally this will result in a singular system after application of the DG method."); |
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.
is that true for a disconnected 1D domain and a disconnected 2D domain?
For example, bunch of TH channels solved with DG connected to a volume solve solved with DG, sharing p, v variables
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.
Hahaha - I was wondering if anyone else was going to consider that case. I thought pointing it out would be too nit-picky, since e.g. it would be natural to use different variables for e.g. v_parallel in a channel vs v_x/v_y/v_z in a volume. But now you've got me thinking about pressure, and it's actually not crazy to have a discontinuous pressure variable that's got the same meaning regardless of element dimension.
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 think we can add means to relax the check if they're needed. But this code in here entirely removes the need for isLowerD code in the framework which was only used for sanity checking, and the MOOSE sanity checking itself was overzealous; like in ProjectionAux there is not a conceptual need for excluding lower-d blocks. The only conceptual need is that all the blocks be of the same dimension which is what this check is doing
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.
Yea, thinking about it more, I think I'm just going to close this and prefer to error in the places that truly can't support a mix of dimensions. No need to an overzealous error check here
Refs idaholab/moose#32003
I think the assertion message could use a little massaging because you could have a defined value that is also discontinuous so discontinuous != undefined