idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.72k stars 1.04k forks source link

Option to bypass mesh generation entirely if RGMB mesh generators are called #25239

Closed shikhar413 closed 2 months ago

shikhar413 commented 1 year ago

Reason

Currently, the SFR workflow project that is part of Griffin is working on a streamlined approach for cross section generation for fast reactor applications. Through this approach, a heterogeneous mesh with region IDs is defined through RGMB is defined as input, and Griffin is able to create an assembly homogenized version of this mesh purely by inspecting the metadata defined on the heterogeneous mesh, according to the specifications laid out in #24198.

However, an issue with the current workflow is that depending on how finely discretized this heterogeneous mesh is, the mesh generation workflow can be slowed down or become too memory intensive, since MOOSE will try to build the heterogeneous mesh, which can quickly number to millions of elements. Since the mesh generator created in Griffin to create the homogenized mesh operates purely on the metadata defined by the heterogeneous mesh, the option to be able to skip the mesh generation step of the heterogeneous mesh is desired.

Design

There are two potential ways to handle this. The first would be to add a flag in RGMB, say skip_generation, that would skip the generate step of the mesh generator workflow and return a pointer to a null mesh instead. Since mesh metadata gets defined through the RestartableData interface, the metadata should still be defined even if a mesh isn't created, but additional logic must be put in place to make sure that another mesh generator like BlockDeletionGenerator, which expects an input mesh, doesn't get appended afterwards and try to access a mesh pointer that has been set to null by RGMB mesh generators. In addition, some metadata gets defined by the base mesh generators in PolygonConcentricCircleMeshGenerator and PatternedHexMeshGenerator, so I'm not sure yet if skipping the mesh generation step will result in missing metadata or metadata being incorrectly defined by RGMB.

After a discussion with @lindsayad, @YaqiWang, and @NamjaeChoi , an alternative approach would be to create a separate class that works similarly to the base MeshGenerator class, but whose role is to do all steps of the MeshGenerator except the actual mesh generation. If we can restrict this special behavior just to this class, then this may make the code more robust to any unforeseen use cases. However, further discussion is needed about what needs to be a part of this class, maybe @lindsayad can outline some of the requirements for this.

Impact

Proper checks should be added to make sure that the MeshGenerator class can handle and cases where a null pointer is returned by RGMB in place of an actual mesh. The ideal situation here with RGMB is to have the option to define metadata only and skip mesh generation entirely

shikhar413 commented 1 year ago

Tagging @GiudGiud, @loganharbour, and @eshemon for their awareness

shikhar413 commented 1 year ago

This functionality would also be useful for @kkiesling as well, since she is working on an interface to convert MOOSE FEM meshes to Monte Carlo geometries, and would be suffering from similar slowdowns and bottlenecks depending on how large her input heterogeneous mesh might be

GiudGiud commented 1 year ago

I'd be interested to see the design of another class that follows the mesh generation process without making the mesh, and without duplicating the metadata generation logic.

All the mesh meta data is declared at the construction of the MG, but some of it may be set during generate() right?

lindsayad commented 1 year ago

For safety I think this logic should be added

diff --git a/framework/src/meshgenerators/MeshGenerator.C b/framework/src/meshgenerators/MeshGenerator.C
index 950f240eb1..ab93307f6e 100644
--- a/framework/src/meshgenerators/MeshGenerator.C
+++ b/framework/src/meshgenerators/MeshGenerator.C
@@ -140,7 +140,10 @@ MeshGenerator::getMesh(const std::string & param_name, const bool allow_invalid
   const MeshGeneratorName * name = getMeshGeneratorNameFromParam(param_name, allow_invalid);
   if (!name)
     return _null_mesh;
-  return getMeshByName(*name);
+  auto & mesh = getMeshByName(*name);
+  if (!mesh && !allow_invalid)
+    mooseError("Returning a null mesh. The previous mesh generator must not have returned a mesh.");
+  return mesh;
 }
lindsayad commented 1 year ago

All the mesh meta data is declared at the construction of the MG, but some of it may be set during generate() right?

If all the mesh meta data is generated at construction, then I think there should be a concrete base class XXXMetaDataGenerator that people can use to generate their meta data without the mesh. And then XXXMeshGenerator could inherit from that class and generate a real mesh for people who want it

shikhar413 commented 1 year ago

If all the mesh meta data is generated at construction, then I think there should be a concrete base class XXXMetaDataGenerator that people can use to generate their meta data without the mesh. And then XXXMeshGenerator could inherit from that class and generate a real mesh for people who want it

@lindsayad I’m thinking more about this but what class would the RGMB mesh generators inherit? Basically a mesh generator like CoreMeshGenerator would be bypassing the mesh generation step based on the presence of some control parameter, but if this control parameter isn’t there then we’d still want CoreMeshGenerator to generate the mesh normally. To me, adding a control parameter directly to MeshGenerator base class would make the most sense, where we would control the workflow for mesh generation based on the presence of this parameter. Otherwise it’s not clear to me which base class CoreMeshGenerator would inherit since there are some cases when we’d want to skip mesh generation but other cases when we don’t want to do this. Let me know if I’m not understanding the proposed workaround correctly

GiudGiud commented 1 year ago

Maybe you would not use the CoreMeshGenerator, you would use the CoreMeshMetaDataGenerator when wanting to only generate the data. CoreMG is a good example for how this is going to be tricky. It doesnt do its own mesh generation, it heavily relies on sub-MGs. So all the subMGs would need to:

I think framework-level changes would be needed indeed, to be able to propagate the modifications (no-mesh generated) to every MG.

I still think the use-case is not that strong. It doesnt take 3 hours to generate a mesh if you reduce the discretization down to the minimum allowed. If something is already doable with simple input file modifications, it has to go the bottom of the priority list. You could add a parameter to CMG for "minimum_discretization" or something to make it light.

What are the generation times for the SFR in the desired configuration? Is there any MG that sets metadata in generate() ?

lindsayad commented 1 year ago

I don’t think @shikhar413 views this as a high priority either but @YaqiWang has been pushing on it. It would seem weird if returning a null mesh or dummy mesh became a canonical practice. It would make it seem as if the mesh generator system isn’t really fitting a user need and it’s being “abused” to achieve a purpose it wasn’t really designed for. The mesh meta data was kind of a bolt on after development of mesh generators and it’s really taken on a life of its own.

Do we need to consider an additional API on mesh generators that returns meta data or is specific to generating meta data? Or is that and the mesh too intricately linked in most cases?

shikhar413 commented 1 year ago

I agree with Guillaume's assessment - since checkpointing with mesh metadata exists, this functionality definitely falls in the category of "helps speed up the workflow" but not blocking the SFR development workflow entirely especially if we use a less finely discretized heterogeneous mesh. I've brought this up with @eshemon and given the other deliverables we have for FY23 this implementation will likely have to wait for FY24 but we can pick this up pretty quickly once the new fiscal year begins.

Do we need to consider an additional API on mesh generators that returns meta data or is specific to generating meta data? Or is that and the mesh too intricately linked in most cases?

In my opinion, currently mesh generation and metadata definition are highly intertwined, and separating out the processes of metadata definition and mesh construction may not be such a trivial task and may require significant code changes. I've taken some steps towards making the distinction between mesh construction and metadata definition (see the definition of generateMetadata in PinMeshGenerator.C, AssemblyMeshGenerator.C, and CoreMeshGenerator.C in the Reactor module, this could be defined as a virtual function within MeshGenerator.C that by default doesn't do anything), however this is not standard practice by the developer and most examples of setMeshProperty and declareMeshProperty are scattered pretty arbitrarily throughout the constructor, so the process of metadata definition is not very sandboxed. This also ignores the fact that metadata can also be defined during mesh generation itself as well, so if we were to skip mesh generation we'd get different metadata definitions than when mesh generation occurs. This might not be an issue in and of itself but we might want to consider the general case where a user might expect certain metadata to exist but not actually be found because mesh generation was skipped.

That being said, one potential implementation design would be to define a control parameter skip_mesh_generation that exists within the MeshGenerator class, which if true will not call generateInternal in MeshGeneratorSystem. This way, RGMB can set this parameter to true through the mesh subgenerator calls, and we just have to add checks throughout the code for MOOSE to error out if another function calls getMesh but mesh generation has been skipped, which can probably be accomplished by using the diff shared by @lindsayad as a starting point. The assumption here is that most global metadata that downstream users would be interested should exclusively be defined in the mesh constructor, and any mesh generation-related metadata would only be defined in generate. Any metadata defined during mesh generation is mostly to assist with mesh generation itself, so the absence of this metadata might not be of a concern to a user who is not interested in mesh generation. Of course, this is just an assumption and it would be preferable to enforce through code that metadata defined during mesh generation should only be done to inform later mesh generators how to build the full mesh, rather than the metadata being used to describe some general geometrical attribute. If we were to go down this route, we would also need to relax some of the requirements where each mesh generator has to "apply an operation" to the input mesh(es) by calling getMesh, since getMesh can now potentially return null pointers.

shikhar413 commented 1 year ago

This is the requirement I'm referring to that would probably have to be relaxed if we go down this road:

You failed to request the generated mesh(es) for the parameter 'input'.

In specific, the mesh from MeshGenerator 'core' was not requested.

To correct this, you should remove the parameter if the mesh(es)
are not needed, or request the mesh(es) with getMesh()/getMeshes().
loganharbour commented 1 year ago

The assumption here is that most global metadata that downstream users would be interested should exclusively be defined in the mesh constructor, and any mesh generation-related metadata would only be defined in generate.

This is a pretty weird requirement in my opinion