-
Notifications
You must be signed in to change notification settings - Fork 503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for issue #3158 - Streamline use of CompositeSurface with SurfaceFilter #3167
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor thoughts for your consideration @zoeprieto.
tests/unit_tests/test_filters.py
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Co-authored-by: Patrick Shriwise <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of additional thoughts on streamlining use of CompositeFilter
.
openmc/filter.py
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature to combine Surface
and CompositeSurface
is now available. And I have also added a user warning.
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, leavingSurfaceFilter
as-is.Additionally the following changes were made:
get_id_surfaces()
to be able to writeCompositeSurface
surface source files.Fixes #3158
Checklist