Introduce new C API function for slice plots#3806
Conversation
|
If any of you want to try this out in the plotter, I've just put up a draft PR there: openmc-dev/plotter#172. |
pshriwise
left a comment
There was a problem hiding this comment.
I think this looks great @paulromano. It should save us a lot of time plotting, particularly for models where find_cell is expensive. I know there's been a lot of desire for off-axis plots as well. Relatively minor comments from me here, but more to say on design and some of the questions you raised.
One limitation I see here that might be problematic is the specification of a single Filter. It would require a fair amount to add support for more than one I know, but if we want to rely on the filter matching in a more general way for plotting then I think we may need to support multiple Filters. (e.g. I often see mesh and cell/material filters used on the same tally).
For the plotter utility specifically it might be additionally beneficial to request the filter matches for all spatial filters at once so that the filter bin map doesn't need to be re-generated when switching tallies if the view itself doesn't change.
As for the id_map and property_map methods, I don't see any reason why we wouldn't deprecate them and have them return the appropriate subset of data from slice_plot for now.
Regarding Model.plot, Model.slice_plot returns raw data about the model (wondering if we might rename that to Model.slice in fact) and Model.plot does a lot of extra work to display an image, so we can keep the plotting bells & whistles there and it can rely on Model.slice_plot for the image data.
| Position origin_; //!< Plot origin in geometry | ||
| Position width_; //!< Plot width in geometry | ||
| PlotBasis basis_; //!< Plot basis (XY/XZ/YZ) | ||
| Position u_span_; //!< Full-width span vector in geometry |
There was a problem hiding this comment.
Perhaps Direction is a more appropriate type for the vectors u_span_ and v_span_ specified here.
| Position u_span_; //!< Full-width span vector in geometry | ||
| Position v_span_; //!< Full-height span vector in geometry | ||
| Position width_; //!< Axis-aligned plot width in geometry | ||
| PlotBasis basis_; //!< Plot basis (XY/XZ/YZ) for axis-aligned slices |
There was a problem hiding this comment.
Is this property still needed on the C++ side?
| # tally with the filter | ||
| filter_ids = {f.id for t in self.tallies for f in t.filters} | ||
| original_length = len(self.tallies) | ||
| if filter is not None and filter.id not in filter_ids: |
There was a problem hiding this comment.
It looks like we can skip the filter set generation if the filter argument is None with a little refactor.
Description
This PR introduces a new
openmc_slice_plotC API function that addresses several limitations of the existing API:openmc_id_map(for cell/material IDs) andopenmc_property_map(for densities/temperatures). Both of these calls involve the same loop over pixels with a "find cell" call at each pixel. In theopenmc_slice_plotfunction, both IDs and properties are retrieved at the same time, which will reduce the time for each slice plot in the plotter by 2×.openmc_filter_get_plot_binsfunction that is similar to the existingopenmc_mesh_get_plot_bins, but once again this would involve another loop over pixels with "find cell" calls (because filters rely on a properly constructed particle with geometry state information). Instead, I've designedopenmc_slice_plotto optionally take a filter index and return matching bin indices for each pixel. This will both speed up tally visualization in the plotter and allow us to plot things we can't currently (likeMeshMaterialFilter).openmc_slice_plotis more general and allows arbitrary orientation slice plots by specifying two orthogonal span vectors that define the view.openmc_id_mapandopenmc_property_mapcalls rely on a struct that has to be created on the Python side matching the C struct internally, which has always felt awkward to me.openmc_slice_plotis designed to take plain basic datatypes to avoid this.Design questions
id_map,property_map, and_PlotBasearound sinceslice_plotsubsumes their capability.Modelclass? Currently I have a separateslice_plotmethod but arguably it can (should?) be integrated into the existingplotmethod?Checklist