Implementation of Triply Periodic Minimal Surfaces (TPMS) in OpenMC

[pferney05]

Description

Hello,

This is my first contribution to OpenMC. I did my best to fit the guidelines but it might not be perfect. I am very open to critics so I can be more efficient in the future. Sorry in advance for potential headaches as it is a big feature.

I am publishing a paper soon that will dive in the technical details of this pull request. Here is a reference to provide some context on TPMS:

https://www.sciencedirect.com/science/article/pii/S014919702300330X

This development introduces two new Surfaces objects:

• TPMS which represent a TPMS surface which can be "Schwarz-P", "Gyroid" or "Diamond" . It has got a pitch and an isovalue coefficient alongside 12 other coefficients who define the translation and rotation of the surface.
• FunctionTPMS which represent a TPMS whose pitch and isovalue are not constant, but interpolated from a 3D matrix.

This development has been made because we initially used meshed TPMS with Serpent2. It works fine for unit-cell cases but with more complexity, the size of files are just too big (several GB) or the mesh is too coarse, which leads to several % of bias on the keff. The performance are not better, but at least we don't have biases as we work with high fidelity surfaces.

To support the distance calculation, an algorithm of ray-tracing has been developed in tpms.cpp. Comments explain the philosophy behind the implementation. This has been tested and verificated against a Serpent + CAD files workflow. The very workflow used in the article above.

The cell.cpp file is updated so TPMS surfaces get a special treatment. First, all non TPMS surfaces are treated to get a first distance estimation. Then the TPMS are treated with the ray-tracing algorithm which takes in input the first distance estimation as an end of loop condition.

The implementation have been made so it will be easy in the future to implement other kind of TPMS or Function. Helper functions to perform all necessary operations have been implemented. a bisection function has been implemented, directly taken from the Boost library.

Fixes # (issue)

Initial implementation. No fixes.

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[gridley]

Here are some preliminary comments on your code. Regardless of the utility of the functionality being introduced, I think there’s room for improvement in how the data is structured. In particular, some existing OpenMC functionality could be leveraged—such as using the XML-to-xtensor reader instead of building custom string-to-matrix readers.

I also recommend reviewing the developer style guide, especially regarding function naming. The use of camel case is inconsistent with our convention, and keeping things uniform improves readability and maintainability.

Lastly, have you considered using mesh-based geometries for these exotic surfaces? Is there a measurable speed advantage to the current approach? Given enough complexity, mesh-based ray tracing might actually be faster than handling intricate ray-surface intersections.

[gridley]

Ideally, the base class shouldn't have to have any methods like this. I believe this is not in line with the "dependency inversion principle" which, if followed, means you'll have cleaner object oriented code. do you know of any other way to accomplish this without letting the base class know we're dealing with a TPMS surface?

[pferney05]

Well, I tried to do a dynamic casting first, like the following:

Surface* surface = /* some initialization */;

// Attempt to dynamically cast surface to TPMS*
TPMS* tpms = dynamic_cast<TPMS*>(surface);

if (tpms) {
    std::cout << "surface is a TPMS" << std::endl;
} else {
    std::cout << "surface is NOT a TPMS" << std::endl;
}

But it felt even more dirty, and it might hurt cases where there are a lot of surfaces. Moreover, we can probably generalize that feature to allow ray-tracing optimization to other surfaces.

Let me know which solution would be best.

[gridley]

ah yeah, you shouldn't need to use a dynamic case. ideally, you would just put virtual methods on the surface class that make it behave correctly. dynamic casting means you have done your abstractions poorly. do you have any ideas about what the general logic at hand here might be?

[pferney05]

Basically, I do the following in the cell.cpp:

• check if the surface is a tpms. If it is, I put its token in a list for later. Otherwise I compute the distance.
• With the distance computed for non-tpms surface I compute the distance for TPMS using the distance of non tpms as a criterium to go out of the ray-tracing loop (going further than that distance is not needed).

[gridley]

We should probably leave this as =0. That prevents programmer mistakes and ensures that base classes implement this method, which will surely be unique for each surface type.

[pferney05]

I had to edit that part because of the special treatment of tpms :

cell.cpp l833

const auto& surface = model::surfaces[abs(token) - 1];

// Calculate the distance to this TPMS surface.
bool coincident {std::abs(token) == std::abs(on_surface)};
double d {surface->distance(r, u, coincident, min_dist)};

I am open to another solution but I always failed to create an object surface which is specifically is a SurfaceTPMS or SurfaceFunctionTPMS.

[gridley]

can you explain why TPMS are treated as a special case in distance calculations? IMO, if there is some generalized logic here like ray tracing using a fast lower bound distance, that could be included as a general feature in the surface class. For example, I have been thinking about implementing a helix surface, where a lower bound on the ray trace distance could also be helpful for determining the intersection.

[pferney05]

You guessed right. TPMS surface have no intrinsic bounding box. So I need to get the lowest distance to any not tpms surface of the cell to have a relevant maximum range parameter for my ray-tracing algorithm. I don't mind changing my implementation to fit a generalized logic.

[gridley]

check the naming convention for functions.

https://docs.openmc.org/en/latest/devguide/styleguide.html#naming

[pferney05]

This function will be deleted in the new implementation of the next commit.

[gridley]

I think you should just implement each as its own class, and not switch based on string comparison in a run-time class like this. building the object on each evaluation, too, might be computationally wasteful depending on what's happening in the constructor.

[pferney05]

I thought about that solution but I was afraid of making surface.cpp hard to read with the addition of at least 6 surfaces with redundant code.

I can do it if this solution is preferred

[pferney05]

Here are some preliminary comments on your code. Regardless of the utility of the functionality being introduced, I think there’s room for improvement in how the data is structured. In particular, some existing OpenMC functionality could be leveraged—such as using the XML-to-xtensor reader instead of building custom string-to-matrix readers.

I also recommend reviewing the developer style guide, especially regarding function naming. The use of camel case is inconsistent with our convention, and keeping things uniform improves readability and maintainability.

Lastly, have you considered using mesh-based geometries for these exotic surfaces? Is there a measurable speed advantage to the current approach? Given enough complexity, mesh-based ray tracing might actually be faster than handling intricate ray-surface intersections.

Thank you very much @gridley for your insightful review. I addressed the straightforward comments in the new commit. It makes the code already much simpler. I also adopted the xml-to-xtensor logic. This development has been made because we initially used meshed TPMS with Serpent2. It works fine for unit-cell cases but with more complexity, the size of files are just too big (several GB) or the mesh is too coarse, which leads to several % of bias on the keff. The performance are not better, but at least we don't have biases as we work with high fidelity surfaces.