Repository navigation
Streamline use of CompositeSurface with SurfaceFilter - #3167
Conversation
pshriwise
left a comment
pshriwise
left a comment
There was a problem hiding this comment.
A few minor thoughts for your consideration @zoeprieto.
| box_model.geometry.root_universe = openmc.Universe(cells=[c]) | ||
|
|
||
| tally = openmc.Tally() | ||
| tally.filters = [openmc.SurfaceFilter(box.get_surfaces())] |
There was a problem hiding this comment.
I think better yet we'd be able to pass a CompositeSurface directly to the SurfaceFilter class, which might require some additional handling in that object.
There was a problem hiding this comment.
Yes, I think it is clearer. It is already done. I thought it was easier to change the SurfaceFilter class than to make a CompositeSurface always return a list of surfaces. Let me know what you think.
Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>
Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>
pshriwise
left a comment
pshriwise
left a comment
There was a problem hiding this comment.
A couple of additional thoughts on streamlining use of CompositeFilter.
| if(type(bins)==list or isinstance(bins, np.ndarray) or | ||
| isinstance(bins, openmc.Surface)): | ||
| bins = np.atleast_1d(bins) | ||
| else: | ||
| bins = np.atleast_1d(bins.component_surfaces) |
There was a problem hiding this comment.
I think ideally a user would be able to provide the CompositeSurface object in the bins argument alongside the other surfaces:
surfaces = [openmc.ZCylinder(r=1), openmc.ZCylinder(r=2), openmc.ZCylinder(r=3)]
prism = openmc.model.RectgangularPrism(7, 7)
surface_filter = openmc.SurfaceFilter(surfaces + [prism])There was a problem hiding this comment.
We'll probably also want to show a warning explaining that many bins will be added for the CompositeSurface if one exists to the user isn't confused about extra bins appearing in the tally results when applying the resulting filter.
There was a problem hiding this comment.
The feature to combine Surface and CompositeSurface is now available. And I have also added a user warning.
There was a problem hiding this comment.
Based on @paulromano's latest, I let's update this warning to a ValueError directing the user to the method for retrieving the surfaces of the CompositeSurface.
paulromano
left a comment
paulromano
left a comment
There was a problem hiding this comment.
I personally don't like the approach of automatically expanding the composite surface into multiple bins, invisible to the user. As the Zen of Python says, "explicit is better than implicit", so I'd rather leave it to the user to decide which of the underlying surfaces they are interested in. In some cases, it doesn't make sense to use all; for example, a one-sided cone has a disambiguation plane that is not really part of the visible surface.
I suggest updating this PR to simply add the component_surfaces that gives the user an easy way to access the underlying surfaces. I would also consider changing the name to primitive_surfaces.
paulromano
changed the title
|
Fair enough, I was dubious about having multiple tally bins appear in the output for a single object passed here. By going through the mechanics of getting the primitive surfaces themselves, the user will understand. I like it. Good point about the disambiguation surfaces, I hadn't considered that for some of the |
paulromano
left a comment
paulromano
left a comment
There was a problem hiding this comment.
I reverted most of the changes here and just left the component_surfaces property. @pshriwise let me know if you're good with this
|
Looks good! |
Co-authored-by: Patrick Shriwise <pshriwise@gmail.com> Co-authored-by: Paul Romano <paul.k.romano@gmail.com>
3 participants
Description
This is a preliminary solution to issue #3158. The solution b) proposed in the issue was chosen:
CompositeSurfaces.get_surfaces()method that returns a list of the objects surface objects to make it simpler to create the desired filter, leavingSurfaceFilteras-is.Additionally the following changes were made:
get_id_surfaces()to be able to writeCompositeSurfacesurface source files.Fixes #3158
Checklist