openmc-dev / plotter

Native plotting GUI for model design and verification
MIT License
46 stars 18 forks source link

Migrating functionality to openmc repo and different approaches #109

Open shimwell opened 1 year ago

shimwell commented 1 year ago

Hi all

I've been exploring the landscape for plotting openmc materials, geometry and meshes.

I've got a few ideas about possible developments to the plotter that I wanted to share. and get peoples thoughts on.

Proposal

Regardless of if we make use of the Geometry.find method I think it makes sense to migrate all data extraction functionality to openmc and then call it from the plotter. This will help keep the code in the plotter minimal and avoid duplication with openmc. If we do this then users have the option of using the plotter and the python API to make batch plots. This issue has come up on the forum and I've put in a PR for slicing regular meshes. I think there are 3 data extraction functions we need (cells, materials, mesh tally result). I think there are also some axis manipulation needed as well but that varies across the plotting tools (imshow is different to contour and plotly is different to mpl).

Questions

Is this totally going in the wrong direction? Is the :snake: python based find method robust enough to rely on? Is it worth moving code to openmc so that it can be shared with API users?

shimwell commented 1 year ago

Tagging @eepeterson for thoughts on this

pshriwise commented 1 year ago
  • bugbear When plotting a just a geometry we currently need to also load up materials. I've started trying to decouple the two with this PR but further PRs are needed if we want to remove this requirement.

I can understand a desire for this and I think it would be feasible. We'd have to display materials only by their ID, but we should still be able to color by material in the plotter if we are able to load the geometry only. We would probably expose a capability in openmc.lib to do that without affecting the main executable.

  • bugbear When launching the plotter it loads up local xml files automatically. If we want to plot geometry or materials or meshtally we need openmc install and the openmc binary. For instance the plotter runs openmc in plot mode then loads up the resulting image. But there are other ways of finding the materials and geometry on a grid, using the openmc.Geometry.find() method I've written two functions that monkey patch openmc.Geometry to allow the materials or cells to be found on a grid and this grid becomes and image. These are not c++ based like the openmc plot but as there is no need to load nuclear data or run openmc the time to plot is comparable. Also the for loop could be parallelised if speed becomes an issue.

I'm afraid this piece would be heading in the wrong direction. I have trouble seeing the Python version competing with the C++ implementation, even if the Python code is parallelized. The plotter used to do exactly this, but we moved it to the C++ version to improve performance, which increased by something like ~20x (partially due to recent improvements to the CSG code).

I think it makes sense to migrate all data extraction functionality to openmc and then call it from the plotter.

Can you elaborate on what you mean by this? Migrate from the C++ code to the Python module?

  • add slicing to the openmc openmc.RegularMesh class

I could see a world where each of our mesh classes in openmc are able to produce a sliced image with some data applied (similar to how we provide datasets to the write_mesh_to_vtk functions). We could leverage that pretty cleanly in the plotter I think.

Extracting the tally data manipulation from the plotter into openmc and then accessing it from the plotter is another matter. The tally data extraction is closely tied to a lot of the plotter data structures and it would be difficult to get those to interface cleanly.

shimwell commented 1 year ago

Thanks for the feedback Patrick, looks like I am indeed a bit of track in some areas. But it looks like a few other areas are agreeable.

Naturally it is nice to load the materials.xml file if present as it has material name and other parts but it could be optional

eepeterson commented 1 year ago

@shimwell thanks for posting this. I agree with Patrick's feedback. I also agree with you that there is likely room for a little more functionality in the OpenMC Python API with regards to data access and simple visualization. I wonder if in the MVC framework of this app it makes sense to have the model portion entirely contained within openmc proper and have this repo just implementing the V+C portions. In the longer term I could also see this app being added to the openmc repo. I know some people have had a lot of issues getting the plotter to install and work correctly and also keeping it up to date with the latest version of openmc. Although I would defer to the higher-ups on that decision. From a user experience perspective I think it could be a huge quality of life improvement.

paulromano commented 1 year ago

In the longer term I could also see this app being added to the openmc repo. I know some people have had a lot of issues getting the plotter to install and work correctly and also keeping it up to date with the latest version of openmc.

I personally don't see moving this into openmc proper making installation any easier -- all it will do is make installation of openmc harder for everyone else because now they are forced to deal with the same GUI problems that users of the plotter run into.

paulromano commented 3 months ago

In response to @shimwell's comment

Naturally it is nice to load the materials.xml file if present as it has material name and other parts but it could be optional

I just wanted to note that the new capability in #131 requires material information if the source definition has a "fissionable" constraint applied.