Cylindrical prism with optional filleted edges #2309

yardasol · 2022-11-23T07:34:52Z

This PR adds a new function, cylindrical_prism, that creates a finite cylindrical prism region with the option to fillet the cylinder's edges

gonuke

This looks like a useful addition @yardasol. I have a few suggestions related mostly to documentation and code reuse/maintainability.

openmc/model/funcs.py

gonuke · 2022-12-02T02:15:53Z

openmc/model/funcs.py

+                      boundary_type='transmission',
+                      upper_edge_radius=0.,
+                      lower_edge_radius=0.):
+    """Get a finite cylindrical prism.


Perhaps more information here about the rounded edges. From reviewing the code, it seems that this option adds what I would call a fillet at each end.

+1 for fillet terminology

openmc/model/funcs.py

gonuke · 2022-12-02T02:21:44Z

openmc/model/funcs.py

+            args['a'] = r - upper_edge_radius
+            args['b'] = upper_edge_radius
+            args['c'] = upper_edge_radius
+            args[axis + '0'] = origin[axcoord] + height/2 - upper_edge_radius
+            tor_upper_max = tor(name='{} max'.format(axis), **args)
+
+            axis_upper_min = plane(axis, 'upper min', height/2 + origin[axcoord] - upper_edge_radius)
+            cyl_upper_min = cylinder(axis, 'upper min', x1, origin[axcoord1], x2, origin[axcoord2], r - upper_edge_radius)
+
+            corners_upper = (+cyl_upper_min & +tor_upper_max & + axis_upper_min)
+
+        if lower_edge_radius > 0.:
+            args['a'] = r - lower_edge_radius
+            args['b'] = lower_edge_radius
+            args['c'] = lower_edge_radius
+            args[axis + '0'] = origin[axcoord] - height/2 + lower_edge_radius
+            tor_lower_min = tor(name='{} min'.format(axis), **args)
+
+            axis_lower_max = plane(axis, 'lower max', origin[axcoord] - height/2 + lower_edge_radius)
+            cyl_lower_min = cylinder(axis, 'lower min', x1, origin[axcoord1], x2, origin[axcoord2], r - lower_edge_radius)
+
+            corners_lower = (+cyl_lower_min & +tor_lower_min & - axis_lower_max)
+


It seems as though there is an opportunity to reuse at least some of this code in a modular way (e.g. a function or loop) rather than cut/paste for the upper/lower. The benefit of this kind of reuse is for consistency/maintainability and not for performance.

paulromano

Thanks for this contribution @yardasol! In addition to the line comments, can you also add this function to the documentation in docs/source/pythonapi/model.rst?

openmc/model/funcs.py

paulromano · 2022-12-06T12:59:36Z

openmc/model/funcs.py

+        cls = globals()['{}Plane'.format(axis.upper())]
+        return cls(name='{} {}'.format(name, axis),
+                   boundary_type=boundary_type,
+                   **{axis + '0': value})


The use of globals() feels a little hacky. I'd rather we import openmc and then get the object from that. Also, if you put the value of the axis first when creating the plane, it will simplify it a bit:

Suggested change

cls = globals()['{}Plane'.format(axis.upper())]

return cls(name='{} {}'.format(name, axis),

boundary_type=boundary_type,

**{axis + '0': value})

cls = getattr(openmc, f'{axis.upper()}Plane')

return cls(value, name=f'{name} {axis}',

boundary_type=boundary_type)

The globals() approach is used in rectangular_prism and hexagonal_prism so these should probably get changed as well.

openmc/model/funcs.py

pshriwise

Just a few smalll comments from me for now, @yardasol. Sorry it's taken me a bit to get to.

One thought that came to mind is the case where the fillet radius is larger than the cylinder radius. This would result in a degenerate torus (a minor radius larger than the major radius). I think we can do transport on those (@paulromano would know for sure), but I'm curious about your thoughts on what the behavior of this function should be in that case. My initial feeling is that if the corner radius is greater than the cylinder radius, we should limit the fillet radius to the radius of the cylinder and display a warning.

openmc/model/funcs.py

pshriwise · 2022-12-05T06:15:02Z

tests/unit_tests/test_geometry.py

+                                               5,
+                                               origin=(-1,1,3))
+    # clear checks
+    assert (-1, 1, 3) in cyl_prism # center


I think it would be beneficial to use the openmc.lib.find_cell method to check that the C++ code is also getting the correct result too since it's what will affect transport.

Great idea!

pshriwise · 2022-12-06T15:21:24Z

openmc/model/funcs.py

+                      boundary_type='transmission',
+                      upper_edge_radius=0.,
+                      lower_edge_radius=0.):
+    """Get a finite cylindrical prism.


+1 for fillet terminology

Co-authored-by: Paul Romano <[email protected]>

…d-cyl-corners

typo fix in comment Co-authored-by: Patrick Shriwise <[email protected]>

yardasol · 2022-12-08T22:44:58Z

Thanks for the reviews everyone! I believe I have addressed everyone's comments.

@pshriwise I added the cell check via openmc.lib like you asked, but I don't think it's behaving correctly. The rest of the test_geometry.py test seems to get skipped after test_cylindrical_prism(). I'll try to take a look at it with a profiler but I'd also like to know your thoughts on my implementation and also on this behavior.

One thought that came to mind is the case where the fillet radius is larger than the cylinder radius. This would result in a degenerate torus (a minor radius larger than the major radius). I think we can do transport on those (@paulromano would know for sure), but I'm curious about your thoughts on what the behavior of this function should be in that case. My initial feeling is that if the corner radius is greater than the cylinder radius, we should limit the fillet radius to the radius of the cylinder and display a warning.

I believe that I enforce the fillet radii must be less than the cylinder radius using the check_less_than() function near the beginning of the function

pshriwise · 2022-12-09T10:57:08Z

I believe that I enforce the fillet radii must be less than the cylinder radius using the check_less_than() function near the beginning of the function

Ah, yeah so you do. Missed it. Sorry about that.

pshriwise · 2022-12-09T11:03:48Z

tests/unit_tests/test_geometry.py

+                       (-2.4, 1, 0.51)]]
+
+    openmc.lib.init()
+    for prism, inside_points, outside_points in zip(prisms, inside_points, outside_points):


There are a few things to address here:

When openmc.lib.init is called, it will read the XML files and initialize OpenMC in memory, so any subsequent XML files that are generated will be ignored. The init and finalize calls should be inside of the loop so OpenMC is started and shutdown for each prism being tested here. I'd even look into the openmc.lib.run_in_memory() context manager too to simplify this further.

The name cell is going to be undefined in the loop here. Might be good to return the cell created from the make_geometry function.

Going to need a boundary condition on at least one surface in the geometry.

Need to wrap the string passed to match in pytest.raises in an re.escape call to ask the regex matching not to treat the parentheses characters as a pattern. I've been hit by that one before.

Okay, got it working!

gonuke

A few more ideas for streamlining the code. I confess some are based on speculation of how things work and incomplete expertise on my part, but looked like opportunities for simplifying things.

gonuke · 2022-12-09T11:51:35Z

openmc/model/funcs.py

+    # Handle rounded corners if given
+    if upper_fillet_radius > 0. or lower_fillet_radius > 0.:
+        # Get torus class corresponding to given axis
+        tor = globals()['{}Torus'.format(axis.upper())]


(Note: this is outside my expertise, but based on design patterns....) don't you want to access this the same way you access planes and cylinders through the OpenMC object?

Also, can this be moved into your create_fillet function?

Yes, and yes!

gonuke · 2022-12-09T11:53:03Z

openmc/model/funcs.py

+                cyl_name = 'upper_min'
+                axial_bound = +plane(axis, 'upper min', coord)
+            else:
+                coord = origin[axcoord] - height / 2 + lower_fillet_radius


Suggested change

coord = origin[axcoord] - height / 2 + lower_fillet_radius

coord = origin[axcoord] - height / 2 + fillet_radius

openmc/model/funcs.py

gonuke · 2022-12-09T12:24:21Z

openmc/model/funcs.py

+        if fillet_lower is not None and fillet_upper is not None:
+            fillet = fillet_lower | fillet_upper
+        elif fillet_lower is None:
+            fillet = fillet_upper
+        else:
+            fillet = fillet_lower
+
+        prism = prism & ~fillet


(Speculation?) If neither fillet is defined then fillet will be None in this last operation. Is that geometry operation well defined if fillet is None?

If it is.... can you just use the fillet_upper and fillet_lower without the above logic to see if they are defined?

Suggested change

if fillet_lower is not None and fillet_upper is not None:

fillet = fillet_lower | fillet_upper

elif fillet_lower is None:

fillet = fillet_upper

else:

fillet = fillet_lower

prism = prism & ~fillet

prism = prism & ~(fillet_upper | fillet_lower)

Are all those operation well defined if one or both are None?

That's a great question. My guess is that it doesn't work that way based on the documentation for the | operator:

class Union(Region, MutableSequence): r"""Union of two or more regions. Instances of Union are generally created via the | operator applied to two instances of :class:`openmc.Region`. This is illustrated in the following example: >>> s1 = openmc.ZPlane(z0=0.0) >>> s2 = openmc.Sphere(r=637.1e6) >>> type(-s2 | +s1) <class 'openmc.region.Union'> Instances of this class behave like a mutable sequence, e.g., they can be indexed and have an append() method. Parameters ---------- nodes : iterable of openmc.Region

but maybe @paulromano or @pshriwise can verify?

If neither fillet is defined then fillet will be None in this last operation. Is that geometry operation well defined if fillet is None?

At least one fillet is guaranteed to exist because this code block is contained within an if statement that requires at least one of uppet_fillet_radius and lower_fillet_radius to be nonzero.

Co-authored-by: Paul Wilson <[email protected]>

paulromano · 2022-12-13T04:01:55Z

@yardasol Now I'm realizing that we already have openmc.model.RightCircularCylinder, which provides the same thing but without fillets. What do you think about just extending that class to support fillets rather than having two different ways to create cylindrical prisms?

yardasol · 2023-01-09T19:00:19Z

@yardasol Now I'm realizing that we already have openmc.model.RightCircularCylinder, which provides the same thing but without fillets. What do you think about just extending that class to support fillets rather than having two different ways to create cylindrical prisms?

This is a good idea. I've been busy doing other things, which is why I added the "on hold" tag, but I'll be able to get back to this ~~soon (sometime this week most likely)~~ after my thesis work is done

yardasol · 2023-06-11T11:23:44Z

Finally got around to dealing with this one. @paulromano @gonuke @pshriwise

docs/source/pythonapi/model.rst

openmc/model/surface_composite.py

yardasol · 2023-07-11T17:15:56Z

@paulromano bump

paulromano

Sorry for the long delay @yardasol! Thanks for refactoring this into the RightCircularCylinder class -- it's looking really good

openmc/model/funcs.py

openmc/model/surface_composite.py

tests/unit_tests/test_geometry.py

Co-authored-by: Paul Romano <[email protected]>

paulromano

Thanks for the quick updates @yardasol. Good to go!

paulromano · 2023-07-15T13:22:18Z

@pshriwise do you have any further requests on this PR?

pshriwise · 2023-07-18T15:07:41Z

@pshriwise do you have any further requests on this PR?

No further requests from me 🚀

paulromano · 2023-07-18T15:53:16Z

@pshriwise Thanks! need you to hit the approve button for this one to land

pshriwise

Thanks @yardasol!

Co-authored-by: Paul Romano <[email protected]> Co-authored-by: Patrick Shriwise <[email protected]> Co-authored-by: Paul Wilson <[email protected]>

yardasol added Geometry Python API labels Nov 23, 2022

yardasol requested a review from paulromano November 23, 2022 07:35

yardasol added 2 commits November 23, 2022 01:38

add cylindrical_prism function

033ecb0

Add unit test for cylindrical_prism

55abbd3

yardasol changed the title ~~Rounded cyl corners~~ Cylindrical prism with rounded edges option Nov 23, 2022

yardasol requested a review from pshriwise November 23, 2022 17:08

Merge branch 'develop' into rounded-cyl-corners

2409d0e

gonuke suggested changes Dec 2, 2022

View reviewed changes

paulromano requested changes Dec 6, 2022

View reviewed changes

pshriwise requested changes Dec 6, 2022

View reviewed changes

yardasol and others added 7 commits December 8, 2022 15:05

Apply @paulromano 's suggestions from code review

69d2bb5

Co-authored-by: Paul Romano <[email protected]>

add fillet terminology

f0d55e3

Merge remote-tracking branch 'origin/rounded-cyl-corners' into rounde…

56be597

…d-cyl-corners

Apply @pshriwise suggestions from code review

0236e4b

typo fix in comment Co-authored-by: Patrick Shriwise <[email protected]>

pep8 fixes

f77f6c1

fix return string

e3c573b

rewrite test_cyl_prism()

2fa0c96

yardasol requested review from paulromano, gonuke and pshriwise December 8, 2022 22:42

yardasol added 2 commits December 8, 2022 17:04

add helper function to reduce code reuse

974c706

Add cylindrical_prism() to the API docs

6ca6f4d

yardasol changed the title ~~Cylindrical prism with rounded edges option~~ Cylindrical prism with optional filled edges Dec 8, 2022

pshriwise reviewed Dec 9, 2022

View reviewed changes

gonuke suggested changes Dec 9, 2022

View reviewed changes

yardasol force-pushed the rounded-cyl-corners branch from 6715de2 to e1d27a9 Compare December 9, 2022 19:01

Use @gonuke's suggestion for simplifying create_fillet()

e1d27a9

Co-authored-by: Paul Wilson <[email protected]>

yardasol requested a review from gonuke December 9, 2022 21:06

yardasol added 2 commits December 9, 2022 15:28

fix unit test

24432ea

simplify import statements

11ab97f

yardasol added the On hold label Dec 28, 2022

yardasol changed the title ~~Cylindrical prism with optional filled edges~~ Cylindrical prism with optional filleted edges Jan 9, 2023

yardasol force-pushed the rounded-cyl-corners branch from f450152 to 84d35a7 Compare June 11, 2023 11:12

yardasol removed the On hold label Jun 11, 2023

yardasol and others added 3 commits June 11, 2023 13:11

move fillet functionality to RightCircularCylinder

84d35a7

add additional surfaces to docstring

896bb1c

Merge branch 'develop' into rounded-cyl-corners

26a21eb

yardasol commented Jun 13, 2023

View reviewed changes

docs/source/pythonapi/model.rst Outdated Show resolved Hide resolved

openmc/model/surface_composite.py Outdated Show resolved Hide resolved

openmc/model/surface_composite.py Outdated Show resolved Hide resolved

openmc/model/surface_composite.py Outdated Show resolved Hide resolved

yardasol and others added 3 commits June 13, 2023 12:54

cleanup

c28ef4a

typo fix

a1f3071

Merge remote-tracking branch 'upstream/develop' into rounded-cyl-corners

2023006

paulromano requested changes Jul 14, 2023

View reviewed changes

Apply suggestions from @paulromano code review

e9cb99c

Co-authored-by: Paul Romano <[email protected]>

yardasol force-pushed the rounded-cyl-corners branch from 080e742 to e9cb99c Compare July 14, 2023 16:17

yardasol requested a review from paulromano July 14, 2023 16:18

paulromano approved these changes Jul 14, 2023

View reviewed changes

paulromano enabled auto-merge (squash) July 14, 2023 17:40

pshriwise approved these changes Jul 18, 2023

View reviewed changes

paulromano merged commit 922b3f9 into openmc-dev:develop Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cylindrical prism with optional filleted edges #2309

Cylindrical prism with optional filleted edges #2309

yardasol commented Nov 23, 2022 •

edited

Loading

gonuke left a comment

gonuke Dec 2, 2022

pshriwise Dec 6, 2022

gonuke Dec 2, 2022

paulromano left a comment

paulromano Dec 6, 2022

yardasol Dec 8, 2022

pshriwise left a comment •

edited

Loading

pshriwise Dec 5, 2022

yardasol Dec 8, 2022

pshriwise Dec 6, 2022

yardasol commented Dec 8, 2022 •

edited

Loading

pshriwise commented Dec 9, 2022

pshriwise Dec 9, 2022

yardasol Dec 9, 2022

gonuke left a comment

gonuke Dec 9, 2022

gonuke Dec 9, 2022

yardasol Dec 9, 2022

gonuke Dec 9, 2022

gonuke Dec 9, 2022

yardasol Dec 9, 2022

yardasol Dec 9, 2022

paulromano commented Dec 13, 2022

yardasol commented Jan 9, 2023 •

edited

Loading

yardasol commented Jun 11, 2023 •

edited

Loading

yardasol commented Jul 11, 2023

paulromano left a comment

paulromano left a comment

paulromano commented Jul 15, 2023

pshriwise commented Jul 18, 2023

paulromano commented Jul 18, 2023

pshriwise left a comment

	coord = origin[axcoord] - height / 2 + lower_fillet_radius
	coord = origin[axcoord] - height / 2 + fillet_radius

Cylindrical prism with optional filleted edges #2309

Cylindrical prism with optional filleted edges #2309

Conversation

yardasol commented Nov 23, 2022 • edited Loading

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulromano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol commented Dec 8, 2022 • edited Loading

pshriwise commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulromano commented Dec 13, 2022

yardasol commented Jan 9, 2023 • edited Loading

yardasol commented Jun 11, 2023 • edited Loading

yardasol commented Jul 11, 2023

paulromano left a comment

Choose a reason for hiding this comment

paulromano left a comment

Choose a reason for hiding this comment

paulromano commented Jul 15, 2023

pshriwise commented Jul 18, 2023

paulromano commented Jul 18, 2023

pshriwise left a comment

Choose a reason for hiding this comment

yardasol commented Nov 23, 2022 •

edited

Loading

pshriwise left a comment •

edited

Loading

yardasol commented Dec 8, 2022 •

edited

Loading

yardasol commented Jan 9, 2023 •

edited

Loading

yardasol commented Jun 11, 2023 •

edited

Loading