fusion-energy / cad_to_h5m

Converts CAD file(s) such as STP and SAT to a h5m file compatible with DAGMC based simulations using the Cubit Python API
MIT License
12 stars 6 forks source link

add implicit complement option #36

Closed LukeLabrie closed 3 years ago

LukeLabrie commented 3 years ago

add option to assign material to the implicit complement (https://svalinn.github.io/DAGMC/usersguide/codes/openmc.html#implicit-complement-materials)

Cubit's “implicit complement” feature detects which surfaces lie on a boundary between a region that is defined in the solid model and one that is not, and assigns a material to the one that is not. Boolean operations on large complex geometries can result in robustness issues. Implicit complement offers an efficient way to define these volumes.

shimwell commented 3 years ago

Interesting, yes I think this feature matches the package well.

Do you have some thoughts for how to implement it, if not I have some ideas below ...

I guess the first step is to identify the graveyard from the list of files in the files_with_tags list

This could be done as the material for the graveyard is called graveyard, but the filename could be anything.

Then I guess the graveyard volume would need to be found from the internal geometry_description dictionary

I guess another argument would be needed to allow the user to specify the material tag to use for the implicit complement.

Then we can use that material tag and the graveyard volume in a F string python command like this

f'group "mat:{implicit_complement_material_tag}_comp" add vol {grave_yard_volume_number}'

tests to see if it works

we could run cad-to-h5m with the implicit_complement_material_tag see to a know string and then use pymoab to see if that string is in the result material tags

LukeLabrie commented 3 years ago

Yes I think that is more or less how I implemented it locally. I think it makes sense to incorporate the cubit command as part of the tag_geometry_with_mats() function, since it's already looping through each file and assigning it a material.

What I did was add another optional key "implicit_complement" to the FilesWithTags dict (similar to what you did for scale). This key takes as its value the material tag (string). Then in the tag_geometry_with_mats() function, I added another if statement to check for an implicit_complement key, and if so it runs the command you have above here.

This allows for the case that the filename is not called graveyard, since it's the user that determines which volume it gets applied to. On that note, I'm not certain that an implicit complement necessarily needs to be applied to the graveyard volume.

shimwell commented 3 years ago

Ah I wonder if there is some confusion about the implicit complement here. I could be wrong here but my understanding is that the implicit complement is the space between cad volumes. It is not defined by any cad one cad geometry, rather it is the void space between them all. By default this empty space is treated as a vacuum and has no material assigned. We are just allowing users to assign a material like air to the space. So we wouldn't need to associate the implicit void material with a stp file / cad volume in the files_with_tag dict. This might also explain the geometry image you posted on the openmc forum.

LukeLabrie commented 3 years ago

I actually wasn't using the implicit complement in the geometry I posted on the openmc forum, but I have been trying to implement it as an approximation/workaround for that part.

Ah ok that sounds right to me. So in that case (like you said) maybe just another argument in the cad_to_h5m() function specifying the implicit complement's material tag? Then the cubit command could be executed right after the graveyard volume is tagged so that the volume number is at hand.

shimwell commented 3 years ago

Perhaps the next stage is to make a feature branch on your fork that does this operation

LukeLabrie commented 3 years ago

I took a stab at this here:

https://github.com/LukeLabrie/cad_to_h5m/tree/LukeLabrie-with_implicit_complement_option

My cubit license conveniently expired today so I haven't been able to fully test it yet, but I should be able to by Monday and can keep you posted. For that reason I didn't submit the PR. Also I'm not clear if the PR here uses my branch or the main branch as the base, so I held off for now. Happy to submit if it makes sense to.

shimwell commented 3 years ago

The branch looks good. We could do with a test adding to the test file to check that the material is correctly appearing in the h5m file.

Perhaps the test can make use of this package to inspect the resulting h5m file to ensure the material tag is inside.

LukeLabrie commented 3 years ago

I added a test to that branch if you want to take a look (https://github.com/LukeLabrie/cad_to_h5m/tree/LukeLabrie-with_implicit_complement_option).

FYI, I also updated the function description for the scaling test (I think it hadn't been updated yet).

I'm still waiting on my cubit license renewal so I can't fully run the tests yet, but I was still getting an HTTP 404 error even before cad_to_h5m was called in the tests. For me, the link to the steel.stp file that's used in some of the tests is broken. (https://raw.githubusercontent.com/fusion-energy/neutronics_workflow/2f65bdeb802f2b1b25da683d13dcd2b29ffc9ed3/example_05_3D_unstructured_mesh_tally/stage_1_output/steel.stp) Any chance this link changed? If not perhaps it's just a local issue.

shimwell commented 3 years ago

Looks good. I can see a few things that I can fix once the PR is open.

Ah yes sorry the link is broken and I can look into that.

Perhaps just add a usage example with the implicit complement to the README.md using markdown and then if you make a PR I can fix a few parts here and there.

LukeLabrie commented 3 years ago

Just submitted the PR -- thanks for walking me through the process! Let me know if I can help further.

shimwell commented 3 years ago

Thanks for adding this feature. I got a bit mixed up while trying to merge it. Then I remembered that packages that use pymoab can't currently be imported at the same time as packages that use cubit. So the dagmc_h5m_file_inspector can't be used in the same script as cad_to_h5m as a hdf5 conflict occurs. So it looks like everything works reasonable well and I can merge that PR.

LukeLabrie commented 3 years ago

Hey Jonathan! FYI, as I started using this feature I incorporated some others locally that made it work more smoothly for my purposes. Mainly:

1.) I added the capability to generate the graveyard automatically given user input for its dimension. This seemed to work better than actually using one that I had made in cad and exported as a .step file (although I'm not sure why).

2.) Cubit offers some other transforms besides scale, namely "move". For me, this was useful to do in cubit, especially if any repetition was required. What I did was group "scale" and "move" under a "transforms" dict. This could very well have been written as a separate argument -- I just thought it was consistent with cubit documentation to have them together that way.

3.) Related to 2.) above, I found that repeated runs of cad_to_h5m would all happen in the same session, thereby duplicating geometries on top of each other. This was solved by a simple reset command.

I've just added these to my forked branch of the repo (https://github.com/LukeLabrie/cad_to_h5m/tree/LukeLabrie-transforms-graveyard). If these features don't seem too out-of-scope and/or you'd be interested in incorporating any of them, let me know and I'd be happy to open a separate issue & put together a PR for you.

shimwell commented 3 years ago

Hi Luke, welcome back

Always keen to improve things.

Automatic graveyards sounds like a good idea. I actually don't make graveyards anymore as I create a CSG surface for the graveyard instead. But we should add that feature anyway, perhaps it could be expressed as a lower left corner and upper right corner in the same manner that OpenMC does 3D regular meshes. This offers a bit more control as users might not have geometry in the center of the model.

For the move and scale changes, let me take a bit of time to understand them

For the addition of the cubit.reset() command that sounds super and I guess that will be the easiest PR to merge in

shimwell commented 3 years ago

@LukeLabrie I think you might have branched from the main branch. Any chance you could branch from develop

LukeLabrie commented 3 years ago

No problem. This one is branched from develop (https://github.com/LukeLabrie/cad_to_h5m/tree/transforms_and_graveyard).

Good point that graveyard might not be centered on the origin, I'll work on updating that.

Would you prefer to see the cubit.reset() addition and the automatic graveyards as separate issues/PRs?

shimwell commented 3 years ago

I am keen on separate PRs if that is ok, perhaps the reset one first as it should be tiny