Skip to content
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

remap_subdomain_ids capability in stitch_meshes #3613

Merged
merged 4 commits into from
Oct 21, 2023

Conversation

roystgnr
Copy link
Member

@roystgnr roystgnr commented Aug 7, 2023

I'm going to try to default it to "true", on the theory that if we don't see a lot of CI failures then this will be a cheap feature to enable by default whereas if we do see a lot of CI failures then this will be an important feature to enable by default.

This catches the failure cases in idaholab/moose#25070 for me. Shame that it also catches cases in MOOSE for me; I'm going to let it rip here to see what the full CI damage is.

@roystgnr roystgnr force-pushed the remap_subdomain_ids branch from 81303ec to 28631dc Compare August 8, 2023 14:34
@moosebuild
Copy link

moosebuild commented Aug 8, 2023

Job Coverage on ce4b2e1 wanted to post the following:

Coverage

4270c3 #3613 ce4b2e
Total Total +/- New
Rate 61.97% 61.99% +0.01% 89.80%
Hits 67456 67500 +44 44
Misses 41395 41396 +1 5

Diff coverage report

Full coverage report

Warnings

  • New new line coverage rate 89.80% is less than the suggested 90.0%

This comment will be updated on new commits.

@roystgnr
Copy link
Member Author

roystgnr commented Aug 8, 2023

Well this is looking nice so far. CI only has the 14 failures I could reproduce locally, and 13 of those failures turn out to be due to a bug in this PR. If I can figure out the 14th then we're in business.

roystgnr added a commit to roystgnr/moose that referenced this pull request Aug 8, 2023
libMesh stitch_meshes is going to start handling MOOSE "subdomain names
are first-class objects, ids might be just autogenerated" behavior
safely, in addition to the old libMesh "subdomain ids are first-class
objects, names are just expected to be matching" behavior.  But in
between those categories of case is "one submesh may be treating names
as first-class objects, another submesh has a matching id without any
name", and we can't figure out any safe thing to do there, so by default
now we'll scream and die when we see it.

So we definitely don't want to see it in CI.

This is all in service of catching bugs like in idaholab#25070, via
libMesh/libmesh#3613
@roystgnr roystgnr force-pushed the remap_subdomain_ids branch from 28631dc to 16d3119 Compare August 9, 2023 14:26
@roystgnr
Copy link
Member Author

Two steps forward, one step back. But those are more than just "we renumbered subdomain ids" exodiffs; in dbg mode we're actually exceeding max subdomain_id_type somehow... looks like that's a test I need to be doing in opt too, whatever the resolution is.

@roystgnr
Copy link
Member Author

we're actually exceeding max subdomain_id_type somehow...

Okay, no "somehow" about it. idaholab/moose#19950 is the culprit. UINT16_MAX - 1? UINT16_MAX - 1 - _peripheral_regions? Once one of those is in the mesh (ncdump of an intermediate AssemblyMeshGenerator mesh tells me it's "65534" in the failing core_hex.i case), we basically have no room (either in this new libMesh code or in MooseMeshUtils::getNextFreeSubdomainID()) to automatically allocate other subdomain ids past the existing range.

Pinging @shikhar413 - is there any way we can just stop using hard coded subdomain ids, and instead get auto allocated ids for these sorts of purposes? There's just not a big enough namespace for every mesh generator to pick its own magic numbers and pray there will be no conflicts.

@shikhar413
Copy link

Possibly, I remember there was a lot of contention over this with @GiudGiud when we were originally developing this capability since we wanted a way to be able to tag elements with a unique subdomain ID to reference where they were in the mesh later on in generate to be able to modify mesh properties and tag any necessary EEIDs, and we chose a namespace specifically large enough to avoid possible collisions with other mesh generators. This topic could use some re-visiting since it has been a few years and now maybe we can be smarter about using EEIDs instead of subdomain IDs to figure out what type of elements (pins, assembly background, or assembly duct?) are we looking to modify the mesh properties of in generate. This will take some time though, and I can't comment right now about whether using auto allocated ids would work instead.

roystgnr added a commit to roystgnr/moose that referenced this pull request Aug 15, 2023
In the long run we need to be using subdomain names here, from the nice
big ~50^32 sized namespace available there, not picking from 2^16 ids
and hoping to avoid conflicts.

In the short run, this at least gives us a little breathing room in
between the hard-coded ids and the default maximum subdomain_id_type
value, so we can autogenerate subsequent ids safely.

I regenerated the gold exodus files by manually search-and-replacing
saved ids in ncdump output, then ncgen'ing the result, to make sure
there weren't any unwanted changes inadvertently slipped in.

Refs idaholab#19950

This isn't quite enough to make MOOSE compatible with
libMesh/libmesh#3613 but it fixes most of the
conflicts for me.
oanaoana pushed a commit to oanaoana/moose that referenced this pull request Oct 19, 2023
libMesh stitch_meshes is going to start handling MOOSE "subdomain names
are first-class objects, ids might be just autogenerated" behavior
safely, in addition to the old libMesh "subdomain ids are first-class
objects, names are just expected to be matching" behavior.  But in
between those categories of case is "one submesh may be treating names
as first-class objects, another submesh has a matching id without any
name", and we can't figure out any safe thing to do there, so by default
now we'll scream and die when we see it.

So we definitely don't want to see it in CI.

This is all in service of catching bugs like in idaholab#25070, via
libMesh/libmesh#3613
oanaoana pushed a commit to oanaoana/moose that referenced this pull request Oct 19, 2023
In the long run we need to be using subdomain names here, from the nice
big ~50^32 sized namespace available there, not picking from 2^16 ids
and hoping to avoid conflicts.

In the short run, this at least gives us a little breathing room in
between the hard-coded ids and the default maximum subdomain_id_type
value, so we can autogenerate subsequent ids safely.

I regenerated the gold exodus files by manually search-and-replacing
saved ids in ncdump output, then ncgen'ing the result, to make sure
there weren't any unwanted changes inadvertently slipped in.

Refs idaholab#19950

This isn't quite enough to make MOOSE compatible with
libMesh/libmesh#3613 but it fixes most of the
conflicts for me.
I'm going to try to default it to "true", on the theory that if we don't
see a lot of CI failures then this will be a cheap feature to enable by
default, whereas if we do see a lot of CI failures then this will be an
important feature to enable by default.
There are still a couple reactor module tests in MOOSE that are failing
with it true, so we'll need to experiment from that end a bit.
@roystgnr roystgnr force-pushed the remap_subdomain_ids branch from 5aaa390 to ce4b2e1 Compare October 19, 2023 21:41
@roystgnr roystgnr merged commit 2861e79 into libMesh:devel Oct 21, 2023
@roystgnr roystgnr deleted the remap_subdomain_ids branch October 21, 2023 15:21
roystgnr added a commit to roystgnr/libmesh that referenced this pull request Oct 23, 2023
I somehow missed getting this commit into libMesh#3613
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants