Preliminaries for NS component wrapping

[joshuahansel]

This PR takes steps towards #23794.

I'm putting this up now, even though it's incomplete, because I want to discuss the overall design/strategy. This PR currently:

• Adds MooseMesh::getBlocksDimension() to get the dimension of a list of subdomains.
• Improves CombinerGenerator to preserve block and boundary names of secondary meshes (not really related, but happened to be needed for testing the above)
• Adds BlockRestricable::blocksDimension() (which pretty much calls the new MooseMesh::getBlocksDimension())
• Uses blocks dimension instead of global dimension in NSFVAction
• Made a template base class for NSFVAction

[joshuahansel]

@GiudGiud @lindsayad Before going further, I wanted to get your thoughts on my last commit, as it sets up the code-sharing strategy between the NS actions and components. In particular, can you review the overall inheritance strategy and see if it makes sense, or if there's something better to use?

I started out trying an interface class and multiple inheritance (e.g., NSFVAction inheriting from Action and NSFVInterface), but I found that I needed to be able get data and function members from the other parent (e.g., Action) in the interface class. I wasn't sure if this was even possible (could I have somehow dynamic_casted to the other parent class?).

I ended up making a base class template NSFVBase<BaseType>, which inherits from BaseType (for NSFVAction, BaseType is Action, and for the corresponding component, BaseType would be FileMeshComponent, for example). Then I have explicit instantiations of various getter methods (mesh, problem, factory, etc.).

Some ugliness to note (not sure if there is alternative): lots of .template blah/->template blah - the biggest offender is that I have to do params.template get<T> everywhere... This apparently became necessary after I added the explicit instantiations of those getter methods.

[GiudGiud]

i think this is really smart.

Since you are templating everything, you ll probably want to instantiate it in the same file, as you cant instantiate it from elsewhere unless you put all the action code in a header.
This means NS needs to pull in THM.

The true test will be making the component of course.

[moosebuild]

Job Documentation on 9ebb777 wanted to post the following:

View the site here

This comment will be updated on new commits.

[joshuahansel]

Since you are templating everything, you ll probably want to instantiate it in the same file, as you cant instantiate it from elsewhere unless you put all the action code in a header.

Templates confuse the hell out of me, so I don't claim to understand any of this, but I thought that we can get away with splitting the declaration and the implementation, as long as we do the explicit instantiation thing:

template class NSFVBase<Action>;

It does at least work for the action. I'm not sure what would be different in this respect for the component.

This means NS needs to pull in THM.

Are you saying this because there will be the explicit instantiation with the component class? I was wondering about this. I wonder if I can split the implementation code into multiple files, with one file in THM, for example, so that we don't have NS requiring THM. Otherwise, if both of the modules depended on one another, it would seem to imply that they just need to be merged, but we should probably avoid this if possible.

The true test will be making the component of course.

Yeah, I'm guessing there will still be tricky things to overcome, but I should be able to find solutions.

[lindsayad]

I don't think we should re-order the NS-THM dependency just so we can put the template code in the .C file instead of the .h

I also like what you've done here @joshuahansel !

[GiudGiud]

Can we include the C file in the component in addition to the h file?

[joshuahansel]

I don't think we should re-order the NS-THM dependency just so we can put the template code in the .C file instead of the .h

I'm confused. How is re-ordering the NS-THM dependency allowing the template code to be in a .C file? I was thinking that any time that there was an explicit instantiation of a non-NS class, it would require THM, regardless of whether in a .h file or .C file. I put the implementation in the .C file because I thought I got that for free since I have the explicit instantiation there (but I don't really understand).

I'll try the idea of having the component explicit instantiation code in THM somehow.

[GiudGiud]

Yeah you're right.
Let s just have both modules depend on each other? Is this disallowed?

[joshuahansel]

Ok, check out my latest push. I moved the explicit instantiation for Action into its own .C file (and had to move the non-explicit implementation into the .h file). I tried with the component too (NSFVBase_FileMeshComponent.C in THM), and it works, which means we don't need NS to depend on THM.

[lindsayad]

I like what you've done with it @joshuahansel

Let s just have both modules depend on each other? Is this disallowed?

It would be a circular library dependency. That doesn't work in computer science

[GiudGiud]

I don't see the component here.
In the header for the component, you have the explicit instantiation of the template, that is implemented in the a .C file still?

and it compiled and ran?
I m used to us having to define all the template code in the header. And last I looked there was no cleaner solution.

[joshuahansel]

I don't see the component here.

I haven't pushed it yet - I was keeping it on a separate branch.

In the header for the component, you have the explicit instantiation of the template, that is implemented in the a .C file still?

I used the same structure as you see for Action here: the explicit instantiation is in its own .C file. However, I've since realized that I don't even need to the explicit instantiations; I can instead just have pure virtual functions for those methods, and thus we can get rid of NSFVBase_Action.C and NSFVBase_FileMeshComponent.C.

Everything is compiling and running so far (just have the new component inheriting from NSFVBase<FileMeshComponent> and creating a mesh without doing anything else).

[joshuahansel]

Ok, just pushed up everything I have so far. I think it should be close now. I made a test using two instances of the new component.

I still need to add the relevant relationship manager for the component. I need to better acquaint myself with the system and figure out how to do this in the THM setting.

[joshuahansel]

@GiudGiud Any idea why my blocks_max_dimension test fails for distributed mesh? It seems like it has nothing to do with my changes, just the mesh generators I used: it complains of missing boundaries in one of my rename-boundary generators.

[GiudGiud]

The boundary must not be present locally

[joshuahansel]

The boundary must not be present locally

Yeah, do you know how this might be fixed? Mesh generators like RenameBoundaryGenerator should work on a distributed mesh, right?

[GiudGiud]

no most mesh generators do not work on distributed meshes. Boundary are even trickier for domain decomposition.

Renaming should have been fine though. It s probably just a zealous check for the boundary existing on every process. Just add a reduction on a "found" boolean and it will work

[joshuahansel]

no most mesh generators do not work on distributed meshes. Boundary are even trickier for domain decomposition.

Renaming should have been fine though. It s probably just a zealous check for the boundary existing on every process. Just add a reduction on a "found" boolean and it will work

Thanks, it looks like the IDs get gathered, but not the names. I'll see what I can do.

[joshuahansel]

Made #24068 for that bug and asked for help from @roystgnr.

[lindsayad]

@joshuahansel I'm going to unsubscribe for now. Please mention me when this is ready for review

[joshuahansel]

@lindsayad I'm stuck with the distributed mesh failure on the new test. To summarize:

• The two instances of the component should be completely independent.
• The failure only occurs when there are two instances of the component, not the single instance situation in the original NS test.
• The convergence tolerance already seems to be about as tight as it can get.
• Using auto-scaling and related options does significantly improve the spread in residual norms, but ultimately does not fix the issue.

[lindsayad]

Going to unsubscribe for now. Please mention me when you want my attention here again 😄

[moosebuild]

Job Coverage on 9ebb777 wanted to post the following:

Framework coverage

	7cf257	#23974 9ebb77
	Total	Total	+/-	New
Rate	85.14%	85.15%	+0.01%	93.75%
Hits	86880	86899	+19	15
Misses	15160	15157	-3	1

Diff coverage report

Full coverage report

Modules coverage

Navier stokes

	7cf257	#23974 9ebb77
	Total	Total	+/-	New
Rate	83.53%	83.60%	+0.07%	96.95%
Hits	10688	10767	+79	1461
Misses	2107	2112	+5	46

Diff coverage report

Full coverage report

Thermal hydraulics

	7cf257	#23974 9ebb77
	Total	Total	+/-	New
Rate	90.24%	90.20%	-0.04%	88.20%
Hits	12456	12540	+84	157
Misses	1347	1363	+16	21

Diff coverage report

Full coverage report

Full coverage reports

Reports

• framework
• chemical_reactions
• combined
• contact
• electromagnetics
• external_petsc_solver
• fluid_properties
• fsi
• functional_expansion_tools
• geochemistry
• heat_conduction
• level_set
• misc
• navier_stokes
• optimization
• peridynamics
• phase_field
• porous_flow
• ray_tracing
• rdg
• reactor
• richards
• scalar_transport
• solid_properties
• stochastic_tools
• tensor_mechanics
• thermal_hydraulics
• xfem

Warnings

• thermal_hydraulics new line coverage rate 88.20% is less than the suggested 90.0%

This comment will be updated on new commits.

[moosebuild]

Job Precheck on 37c3d98 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/23974/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format e84297e188dde47ffd658f7e933959f4d203c443

[joshuahansel]

@lindsayad @GiudGiud This is ready for review.

[GiudGiud]

looks good

[joshuahansel]

@GiudGiud I think your comments should be addressed now. @lindsayad do you want to review as well?