Skip to content
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

Migrating functionality to openmc repo and different approaches #109

Open
shimwell opened this issue Feb 2, 2023 · 6 comments
Open

Migrating functionality to openmc repo and different approaches #109

shimwell opened this issue Feb 2, 2023 · 6 comments

Comments

@shimwell
Copy link
Member

shimwell commented Feb 2, 2023

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.

  • 🐛🐻 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.

  • 🐛🐻 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.

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).

  • add slicing to the openmc.geometry class for materials and cells that uses openmc.geometry.find function
  • add slicing to the openmc openmc.RegularMesh class
  • make use of both functions here in the plotter and remove local versions

Questions

Is this totally going in the wrong direction?
Is the 🐍 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
Copy link
Member Author

shimwell commented Feb 2, 2023

Tagging @eepeterson for thoughts on this

@pshriwise
Copy link
Collaborator

pshriwise commented Feb 2, 2023

  • 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
Copy link
Member Author

shimwell commented Feb 2, 2023

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.

  • 2D mesh slicing methods in openmc python API ✔️
  • removal of mandatory need to load a materials.xml file ✔️

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants