-
Notifications
You must be signed in to change notification settings - Fork 18
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
Comments
Tagging @eepeterson for thoughts on this |
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
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).
Can you elaborate on what you mean by this? Migrate from the C++ code to the Python module?
I could see a world where each of our mesh classes in Extracting the tally data manipulation from the plotter into |
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 |
@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. |
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. |
In response to @shimwell's comment
I just wanted to note that the new capability in #131 requires material information if the source definition has a "fissionable" constraint applied. |
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).
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?
The text was updated successfully, but these errors were encountered: