Constructive Solid Geometry (CSG) base framework

[kkiesling]

Refs #29095

co-author: @shikhar413

Additional information

This initial PR provides the new CSG framework through the CSGBase class and associated classes to support adding the generateCSG method to mesh generators. The generateCSG method has not been implemented in any existing mesh generators except for those we created for testing purposes, though the instructions for how to implement are throughly documented.

The CSGBase framework currently supports:

• surfaces (planes, spheres, cylinders)
• regions created using operators
• cells (filled with void, material (referenced by name), or universe)
• universes
• passing and joining CSGBase instances with other mesh generator instances
• exporting to a JSON file.
• using --csg-only at the command line to prompt this workflow

Not yet supported with this PR, but will come in subsequent PRs, is support for lattices, transformations, and engineering units.

[kkiesling]

Tagging for awareness: @eshemon @tarapandya @aprilnovak @sallustius

If anyone knows the usernames for Tarek Ghaddar, Matt Jessee, Ishita Trivedi, or Josh Smandych, can you please tag them

[shikhar413]

@GiudGiud @loganharbour can you activate these tests please?

[kkiesling]

It seems that the tests need to be activated with each new push. Can you please activate again @GiudGiud @loganharbour ?

[loganharbour]

It seems that the tests need to be activated with each new push. Can you please activate again @GiudGiud @loganharbour ?

Done. Accept the invitation I just sent and you'll get automatic activation

[GiudGiud]

@ishita-trivedi @joshua-smandych for awareness.

I don't have the others' git handles

[moosebuild]

Job Documentation, step Docs: sync website on 1d15f4d wanted to post the following:

View the site here

This comment will be updated on new commits.

[kkiesling]

Thanks for the quick turn around on an initial review @GiudGiud !

I think we might want to agree on some of the terminology. I would rather CSG base and CSG objects than CSG mesh

CSGBase for the class (or CSGBase object). And yes, CSG objects rather than CSG mesh. We must have missed taking mesh out a few places, we are in agreement on terminology.

[kkiesling]

Thanks for the reviews @GiudGiud and @loganharbour - We had a few follow up questions/comments and may have more as we actually start making updates, but otherwise we will get on making these changes!

[bwspenc]

I just found out about this PR, and am really interested in a CSG capability for use within MOOSE for doing things like defining things like composite models that have multiple materials. I envision using CSG to determine whether a point is inside or outside a collection of spheres or rods, for instance. We have done this sort of thing for simple geometries by defining MOOSE Functions that give a minimum signed distance from the surface of a collection of rods or spheres. I don't see anything quite like that in this PR, but would it be feasible to write a MOOSE Function that uses your CSG objects to determine a signed distance from the surface of an object you define with CSG?

[shikhar413]

@bwspenc thanks for getting in touch. This PR focuses on creating the infrastructure to define an equivalent CSG geometry from a MOOSE mesh input file. Since the CSGBase class (CSG equivalent to a MeshBase class) stores all the surfaces defined within a geometry, it should be possible to define a virtual function within CSGSurface to return the signed distance from a point to the surface, though that hasn't been our focus as of right now. While we don't have the ability to compute signed distances in this PR, we do provide the ability to determine if a point is in the positive or negative halfspace of a surface (see virtual CSGSurface::Direction directionFromPoint). Since this is all defined in memory, this should hopefully allow other things like MOOSE Functions to be able to retrieve CSG-related info.

We haven't thought of all the ways this MOOSE-to-CSG work could be integrated with existing MOOSE capabilities, but we are gathering ideas on areas for future development. We do also hope that once we get some of the basic infrastucture in place here then this will allow others to add additional functionality as they see fit.

[moosebuild]

Job Coverage, step Generate coverage on 1d15f4d wanted to post the following:

Framework coverage

	236997	#30860 1d15f4
	Total	Total	+/-	New
Rate	85.93%	85.98%	+0.05%	100.00%
Hits	122919	123645	+726	770
Misses	20122	20157	+35	0

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

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

This comment will be updated on new commits.

[kkiesling]

This should be ready for review again @GiudGiud @loganharbour (assuming CI passes). I believe we addressed everything either by making the change or providing a comment, except for two things: handling surfaces and discussion of materials. I resolved all the simple/clear comments but left any with discussion on them for reviewers to resolve.

Regarding materials: I am not sure if/how you want us to change anything here right now or if you would prefer we wait on that.

Regarding surfaces: @shikhar413 and I think we understand what you are asking for @GiudGiud but we want to run the approach past you first before we make the major change.

Proposed Surface Changes

Right now, we have CSGBase controlling the creation of each surface, but as you point out, that is not necessary because of how the method exactly mirrors the methods in CSGSurfaceList. This also restricts the type of surface that can be created. With the current implementation, there is not anything stopping a developer from calling CSGSphere or other surface constructor directly to create an orphaned CSGSurface pointer not connected to a CSGBase object.

We are thinking of exposing the addSurface() method that currently only exists in CSGSurfaceList in CSGBase so that an independently created surface could be added to the base instance (what you suggest I think). This would allow a developer to create any sort of custom surface definition in a module or app that inherits from CSGSurface and then add those surfaces directly to their base. To support this change, we would remove all the create[Surface] methods from base and expect that developers call surface constructors directly and then add them explicitly to the base object.

For example, this existing code would be replaced with the code block below:

const auto & sphere = csg_obj->createSphere(name, center, radius);

replacement:

std::unique_ptr<CSG::CSGSurface> surf_ptr = std::make_unique<CSG::CSGSphere>(name, center, radius);
auto & csg_plane = csg_base->addSurface(surf_ptr);

This would also require the change from using the MooseEnum type for _surface_type and gather the surface type directly from the method name (also as you suggested).

Questions about this approach:

1. Is there a better more user/developer-friendly way that we could create a surface object that doesn't require the developer to know to use std::make_unique? Ie, allow them to call auto surf = CSG::CSGSphere(name, center, radius); and add that instead to the base?
2. What do you think about removing all the create[Surface] methods that currently exist in CSGBase? Removing them would enforce consistency in how surfaces are created and implemented throughout. But letting them exist also doesn't harm anything since they are just convenience functions at that point right? The downfall of leaving them is that it would be easy for these convenience methods to become out of sync as different types of surfaces are added.

If this surface change approach is not actually what you were thinking or if I have confused you with this long message, let's setup a meeting to discuss further.

[GiudGiud]

Regarding materials: I am not sure if/how you want us to change anything here right now or if you would prefer we wait on that.

I think we should be able to move forward by not mentioning them for now OR mentioning in a specific section of the documentation how to do 'materials' for the downstream MC codes.

To support this change, we would remove all the create[Surface] methods from base and expect that developers call surface constructors directly and then add them explicitly to the base object.

I like the proposed design. I think it would work for downstream apps being able to implement new CSG surfaces without modifying moose.

Is there a better more user/developer-friendly way that we could create a surface object that doesn't require the developer to know to use std::make_unique? Ie, allow them to call auto surf = CSG::CSGSphere(name, center, radius); and add that instead to the base?

auto surf = CSG::CSGSphere(name, center, radius); is creating the object within a certain scope, and would be deallocated when you leave that scope. I agree it's a little clearer, but it does not work for us. On the other hand the unique pointer, once stored in the CSGSurfaceList, gives the memory ownership. There is a fair bit of usage for make_unique in moose so it's not unreasonable for moose-app developers to know how to it.

What do you think about removing all the create[Surface] methods that currently exist in CSGBase? Removing them would enforce consistency in how surfaces are created and implemented throughout. But letting them exist also doesn't harm anything since they are just convenience functions at that point right? The downfall of leaving them is that it would be easy for these convenience methods to become out of sync as different types of surfaces are added.

I would delete them. Having a unified API is best because any issue will get caught by the multiplicity of cases. New functionality can be implemented in a single place too. Theoretically, a routine per surface could each require a modification when implementing new functionality.

[kkiesling]

Regarding materials: I am not sure if/how you want us to change anything here right now or if you would prefer we wait on that.

I think we should be able to move forward by not mentioning them for now OR mentioning in a specific section of the documentation how to do 'materials' for the downstream MC codes.

@GiudGiud sorry we are still not clear on what you want here. Do you want us to remove the "material" type of cell fill entirely from the code, or just remove mention of it in the documentation? Or should we just use a new name (like is suggested here)?

Alternatively, I could add this note to the documentation to clarify:

A cell with a material fill is not connected to a MOOSE material definition at this time. The "material" is currently just a string to represent the name of a material or other type of fill that is otherwise undefined.

In the long run, we plan to connect the concept of a material to an actual MOOSE material, so for the time being this is really just a placeholder. However, that work isn't planned for a while out because it doesn't seem crucial to get the geometry component off the ground. Hence using the string name as a placeholder.

[GiudGiud]

Or should we just use a new name (like is suggested https://github.com/idaholab/moose/pull/30860#discussion_r2174071735)?

This could be a good idea. Basically those cells are the "physical" cells as opposed to cells filled with universes. Maybe AI tools will have suggestions on alternative names that convey the same impression.

In the long run, we plan to connect the concept of a material to an actual MOOSE material, so for the time being this is really just a placeholder. However, that work isn't planned for a while out because it doesn't seem crucial to get the geometry component off the ground. Hence using the string name as a placeholder.

Agree we don't want to delay because of this. Since there is more than one material per cell in moose it is tough to link the two. While it does work great for Monte Carlo neutronics. But already Griffin/neutronics-in-MOOSE does not use a very simple subdomains-per-material but rather element ids / depletion ids regions on top of block-restriction of materials. So subdomains is not the right concept either

Maybe 'material_region' ? It has this idea that it's not just 1 material. And region is understandable in the context of depletion id regions within subdomains.

I ll ask at our Monday meeting

[shikhar413]

I’m thinking more about this and one complication from a general MOOSE mesh input standpoint is that it isn’t clear what the CSG should be tracking in order to define unique “material regions”. For example, are we defining materials by unique subdomain IDs/names? Are we defining subdomain IDs/names and then mapping them to a material_id EEID? There’s a number of ways to define material zones, and in the current way where we define CSG one MG at a time, there’s some ambiguity about how these CSG material cells should be defined. Then, we have RGMB-like MGs that define subdomains and multiple EEIDs concurrently, which one should we be tracking to define the mesh-material mapping?

If we take this concept one step further and want to connect the CSG to some downstream tally system, maybe we want to keep track of other EEID mappings, such as tallying over each assembly location. Then how do we convey this information to the MOOSE-to-CSG converter on the implementation side as well as the input side? These are some things that I’m thinking about that I don’t have a clear answer to.

I’m fairly confident the work Kalin and I have done so far can define a CSG geometry in a way that can be understood by the downstream MC code. However, I think we should also be pretty clear on how this connects to other aspects of an MC simulation (material zone definitions, material-composition mapping, tallies, maybe some way to keep track of runtime settings related to MC codes?), aspects that may be relevant to MOOSE simulations, and target applications here (are we targeting steady-state MC simulations, does it make sense to try to extend this further into depletion and/or multiphysics and how much support are we providing to the end user?)

[kkiesling]

I think these are all great points. I very much anticipate the handling of this specific part of the CSG framework to have to change when we get to that point, so right now the goal is just "don't confuse people and have something to be a placeholder." I will update the name to "material_region" (or something similar) and also add the note to the documentation. I feel like that's the best compromise for the interim until we get to something more robust. We can also add additional Cell types later too if we want to support these various other considerations.

FWIW, in all MC codes that I am aware of, any change in homogenous material requires a surface. So with depletion zones, those are actually defined by a new surface. And a larger area of two materials will always be divided by a surface where the material boundary is, or the materials will be homogenized. For cells that might have different zones, I foresee this being handled by replacing the material fill of a cell with a universe fill that is a collection of smaller cells representing each zone. And in the other case, we would have to enforce some level of homogenization.

[GiudGiud]

If we take this concept one step further and want to connect the CSG to some downstream tally system,

Right that is part of the problem. We likely want to import fields from tallies into MOOSE so we would want to use the same indexing. Too many indexings! And likely more than one valid per simulation. Maybe we should be able to output more than 1 index per "material" cell.

does it make sense to try to extend this further into depletion and/or multiphysics and how much support are we providing to the end user?)

both will likely happen, even if we don't do it ourselves.

to have to change when we get to that point, so right now the goal is just "don't confuse people and have something to be a placeholder."

Yes that's a reasonable consideration!

We can also add additional Cell types later too if we want to support these various other considerations.

Yes that is also a good idea.

FWIW, in all MC codes that I am aware of, any change in homogenous material requires a surface.

Agree and we capture that in this CSG implementation well.

I foresee this being handled by replacing the material fill of a cell with a universe fill that is a collection of smaller cells representing each zone.

I think it might just be a matter of specifying the same "ID" (not a unique ID concept, just like a mesh element has a subdomain id and arbitrarily many extra element ids") to homogenize.

My guess for now is that we will need to at least have both a cell id and a material id, just like MC codes have, and at first let the MC code handle the various other indexing (which could be grouping ids together). For using these MC codes' indexing when reading their tally output, we might need to either:

• write specific readers when not coupling in memory (like Cardinal, where we can query indices from OpenMC as we are parsing its tally output)
• store and output "extra" IDs on the CSG, and be able to match them programmatically to elements when running a MOOSE simulation that consumes tallies from a MC simulation that used these extra-Ids for tallying

[GiudGiud]

I met with the INL Griffin team. They overall agreed with the idea that this unique material ID used in CSG (+ cell id) by MC codes will best be replaced by a "map" / an option for multiple IDs. That way, the ExtraElementIDs used by Griffin (for example, pin_id, depletion_id, material_id, composition_id ...) can all be output for each cell (with some cells having the same values for a given id-type when homogenizing).

Ideally the downstream readers can then parse this multitude of IDs and make them available for specifying materials, tallies, ....

EDIT: current placeholder is fine as long as you are not seeing an issue with what I am pushing for above. If you find a better name for the concept now that we think it will end up being some sort of map of ids on each cell, please feel free to update!

[shikhar413]

@GiudGiud, Kalin and I just finished talking about your proposed idea. What we are thinking right now is to allow API developers to pass in an optional map of [extra_element_id_name, extra_element_id_value] pairs to CSGCell when it has a CSG_MATERIAL fill (the material fill name and map can probably be stored through a separate class CSGMaterialFill or the like). This map would store all the relevant EEID name and values for that particular material fill, in order to keep track of all possible EEID's that could be mapped to the CSG object. Is that in line with what you had in mind?

We are thinking of keeping the code as is as a placeholder and implementing this new map functionality for EEID's in a future PR when we define the generateCSG method for the RGMB mesh generators, since the map will become relevant at that stage. Are you on board with this plan? If so, could you review this PR in its current form again and let us know if this is good to go? The current implementation should not prevent this future work in any significant way.

[GiudGiud]

Is this a tough one to rebase (lots of conflicts)? If so merge commit is fine but if not rebasing is preferred

[kkiesling]

Is this a tough one to rebase (lots of conflicts)? If so merge commit is fine but if not rebasing is preferred

@GiudGiud I did the merge because of the large media conflict. It gave me a lot of issues with trying to rebase in the past but was ultimately doable. I am not opposed to rebasing, but I am also honestly not sure how to undo a merge at this point..

[kkiesling]

Everything should be addressed now, so this is ready for review again @GiudGiud @loganharbour

[GiudGiud]

It's good to go for me. @roystgnr @loganharbour @lindsayad this is the PR I mentioned if you want to take a look

[GiudGiud]

The CSGBase (and surface and etc) stuff may not need the mesh generator system classes to exist to be created in unit tests

We don't need to be unit testing mesh generator creating CSGs so we hopefully don't need to be creating mesh generators in unit tests

[kkiesling]

@loganharbour - this should be ready for you to review again and/or respond to discussions.

We significantly refined the tests and moved almost everything out into unit tests. @shikhar413 had some responses/questions to some of your code change suggests that warrant some discussion before we make updates.

[kkiesling]

I can't see what the failed job results are for the Controlled apps job on CI ("failed but allowed" it says). But I assume this is the job that is preventing the remaining 8 jobs from running. Can someone who can see the results let me know if this is something on our end to fix, or restart the job so that the rest of them run if it is a random failure?

[moosebuild]

Job Coverage, step Verify coverage on 8877521 wanted to post the following:

The following coverage requirement(s) failed:

• Failed to generate chemical_reactions coverage rate (required: 92.0%)
• Failed to generate combined coverage rate (required: 37.0%)
• Failed to generate contact coverage rate (required: 87.0%)
• Failed to generate electromagnetics coverage rate (required: 94.0%)
• Failed to generate external_petsc_solver coverage rate (required: 84.0%)
• Failed to generate fluid_properties coverage rate (required: 84.0%)
• Failed to generate fsi coverage rate (required: 85.0%)
• Failed to generate functional_expansion_tools coverage rate (required: 81.0%)
• Failed to generate geochemistry coverage rate (required: 96.0%)
• Failed to generate heat_transfer coverage rate (required: 87.0%)
• Failed to generate level_set coverage rate (required: 85.0%)
• Failed to generate misc coverage rate (required: 32.0%)
• Failed to generate navier_stokes coverage rate (required: 77.0%)
• Failed to generate optimization coverage rate (required: 86.0%)
• Failed to generate peridynamics coverage rate (required: 77.0%)
• Failed to generate phase_field coverage rate (required: 85.0%)
• Failed to generate porous_flow coverage rate (required: 95.0%)
• Failed to generate ray_tracing coverage rate (required: 93.0%)
• Failed to generate rdg coverage rate (required: 63.0%)
• Failed to generate reactor coverage rate (required: 90.0%)
• Failed to generate richards coverage rate (required: 93.0%)
• Failed to generate scalar_transport coverage rate (required: 81.0%)
• Failed to generate solid_mechanics coverage rate (required: 84.0%)
• Failed to generate solid_properties coverage rate (required: 83.0%)
• Failed to generate stochastic_tools coverage rate (required: 88.0%)
• Failed to generate subchannel coverage rate (required: 87.0%)
• Failed to generate thermal_hydraulics coverage rate (required: 88.0%)
• Failed to generate xfem coverage rate (required: 80.0%)

[kkiesling]

Hi @loganharbour, pinging this again so you can take a look at the remain questions and comments, and review again if necessary. Thanks!

[loganharbour]

Hi @loganharbour, pinging this again so you can take a look at the remain questions and comments, and review again if necessary. Thanks!

Added to my list and will get to this tomorrow 👍🏼