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

added material.from_library method #2105

Open
wants to merge 34 commits into
base: develop
Choose a base branch
from

Conversation

shimwell
Copy link
Member

@shimwell shimwell commented Jul 5, 2022

Hi all,

This PR attempts to add the ability to add material composition and density from a predefined collection / library.

This PR follows on from this discussion where adding materials from the PNNL library of predefined materials was discussed.

Adding a PNNL_v1 material database has previously be performed by PyNe and more recently by ATTILA. I've also done something similar before in the neutronics-material-maker package.

One of the differences to existing implementations and this PR is that we have revision version 2 (released in 2021) of the PNNL compendium released in here instead of the first revision (released in 2011)

I think this method for this implementation is quite concise due to expanding the dictionary into keyword arguments with ** . The implementation is also extendable as the JSON file can support multiple libraries if PNNL_v3 gets release or there is a need to add another material compendium.

I've also added a simple test to the test_material.py unit test but the usage is simple

>>>import openmc

>>>m1 = openmc.Material()
>>>m1.add_from_library(name='Sodium Oxide', library='pnnl_v2')
>>>m1
Material
	ID             =	1
	Name           =	
	Temperature    =	None
	Density        =	2.27 [g/cc]
	S(a,b) Tables  
	Nuclides       
	O16            =	0.332523     [ao]
	O17            =	0.0001266665 [ao]
	O18            =	0.0006833327 [ao]
	Na23           =	0.666667     [ao]

@shimwell shimwell requested a review from paulromano as a code owner July 5, 2022 16:21
@eepeterson
Copy link
Contributor

@shimwell I really like this! My first bit of feedback is that I think having a single json file for each library may be clearer and I think the method should be a class method on the Material class so something more like this: m1 = openmc.Material.from_library(name='Sodium Oxide', library='pnnl_v2')

@shimwell
Copy link
Member Author

shimwell commented Jul 6, 2022

Thanks for the feedback @eepeterson I've moved the method to a classmethod as suggested.

The python example from the first comment now changes to

>>>import openmc

>>>m1 = openmc.Material.from_library(name='Sodium Oxide', library='pnnl_v2')
>>>m1
Material
	ID             =	1
	Name           =	
	Temperature    =	None
	Density        =	2.27 [g/cc]
	S(a,b) Tables  
	Nuclides       
	O16            =	0.332523     [ao]
	O17            =	0.0001266665 [ao]
	O18            =	0.0006833327 [ao]
	Na23           =	0.666667     [ao]

@shimwell
Copy link
Member Author

shimwell commented Jul 6, 2022

@eepeterson I've refactored the PR to use a single files for each library as suggested 👍 . I guess this will be handy when adding private material libraries.

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @shimwell! One question I have is how the JSON file is being generated. I see that in the material compendium in many cases they have elemental compositions, but in the JSON file we have nuclides explicitly. This will create problems if the data library being used doesn't have a particular nuclide. For example, Anthracene uses C12 and C13, which won't work with ENDF/B-VII.1 and earlier. Is it possible to switch over to using elemental compositions when available?

@paulromano
Copy link
Contributor

@shimwell Also looks like you have a conflict on material.py

@eepeterson
Copy link
Contributor

@paulromano and @shimwell I have been thinking about this a little lately and I think this might warrant just a little more structure to do robustly, namely a MaterialLibrary class with to/from file methods, the ability to handle elemental compositions in addition to nuclide compositions, as well as the ability to merge existing materials.xml files into a library, and nice printing/repr for QA purposes. I also think an optional environment variable like OPENMC_MATERIAL_LIBRARY_PATH as a colon separated list of paths (like $PATH) would be powerful and separate any internally stored libraries like the PNNL_v2 one in this PR from other custom libraries. All of these features are pretty straightforward, the only question in my mind is if we want the JSON format for libraries or if there is another preferred format. Thoughts?

@shimwell
Copy link
Member Author

I was just looking at a nice PR #2139 . Once #2139 is merged in then this PR could be refactored to make use of the new methods introduced. The JSON file could also be reduced in side considerably to dictionaries of tuples instead of the current rather verbose dictionary.

Copy link
Contributor

@eepeterson eepeterson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first round of small comments

openmc/data/data.py Outdated Show resolved Hide resolved
openmc/material.py Outdated Show resolved Hide resolved
openmc/material.py Outdated Show resolved Hide resolved
openmc/data/data.py Outdated Show resolved Hide resolved
Co-authored-by: Ethan Peterson <ethan.peterson@mit.edu>
Copy link
Contributor

@eepeterson eepeterson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a little refactoring here so that we can use external libraries as well based on an environment variable

openmc/material.py Show resolved Hide resolved
openmc/material.py Outdated Show resolved Hide resolved
openmc/material.py Outdated Show resolved Hide resolved
openmc/material.py Outdated Show resolved Hide resolved
openmc/material.py Outdated Show resolved Hide resolved
@eepeterson
Copy link
Contributor

@shimwell I think we need to move the library file to material_libraries/pnnl_v2.json to fix the failing test

@shimwell
Copy link
Member Author

shimwell commented Aug 5, 2022

@shimwell I think we need to move the library file to material_libraries/pnnl_v2.json to fix the failing test

Thanks Ethan. I've created the folder, renamed the file and moved it in there. I've also added it to the package data in setup.py. Hoping the tests pass this time 👀

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really handy capability. I spent a good chunk of time translating material definitions from the PNNL compendium by hand when learning to use MCNP. Thanks @shimwell and @eepeterson for working on this!

Just a few small thoughts/comments from me.

openmc/material.py Outdated Show resolved Hide resolved
openmc/material.py Outdated Show resolved Hide resolved
openmc/material.py Show resolved Hide resolved
openmc/material.py Outdated Show resolved Hide resolved
openmc/material.py Outdated Show resolved Hide resolved
tests/unit_tests/test_material.py Outdated Show resolved Hide resolved
@shimwell
Copy link
Member Author

shimwell commented Aug 10, 2022

I came across a little error that the CI didn't catch. Perhaps because we use pip install -e . which links but doesn't copy files

when using pip install . the pnnl_v2.xml file was not being copied over to the directory where python package was installed.

So I've added an __init__.py file to the openmc/data/material_libraries folder that helps the setup.py realize that these xml files should be copied

openmc/material.py Outdated Show resolved Hide resolved
@shimwell
Copy link
Member Author

shimwell commented Dec 8, 2022

I have left this PR for a while as the plan was to refactor the materials class so that is accepts elements then come back to this PR.

However I have an alternative suggestions.

How about we remove the openmc/data/material_libraries/pnnl_v2.xml file and the code that adds the internal library so we are no longer adding a specific PNNL library to the code. However we retain the functionality of adding a users xml material library file?

basically just retaining part of this PR so that users can load in their own material library.

This idea was inspired by @mwart-cfs who has a use case I had not considered. For QA purposes they prefer no material libraries to be initially loaded into the code but want the ability to add user libraries. Further details here

@eepeterson
Copy link
Contributor

I don’t quite see how not including widely used libraries within OpenMC directly helps with QA exactly and to me it doesn’t make sense for something like the PNNL compendium to exist within every users custom libraries. Since properly including elements in material specifications does require significant refactoring, maybe it is worth implementing a temporary way of including user libraries, but im not sure.

@shimwell
Copy link
Member Author

shimwell commented Dec 8, 2022

How about we break this PR into two parts.

Part 1, just the frame work to allow users to load in their own custom libraries (xml files)

Part 2, after the material refactor add in the PNNL compendium so that it is distributed with openmc package and available as a default loaded in library

@shimwell
Copy link
Member Author

I think this might be worth another go as the materials compendium is now downloadable in json format https://compendium.cwmd.pnnl.gov/

@shimwell
Copy link
Member Author

Also there is now a Pyne materials package that creates pyne materials from the compendium. Nice work by @ahnaf-tahmid-chowdhury

https://github.com/pyne/materials-compendium

@MicahGale
Copy link
Contributor

I came across a little error that the CI didn't catch. Perhaps because we use pip install -e . which links but doesn't copy files

when using pip install . the pnnl_v2.xml file was not being copied over to the directory where python package was installed.

So I've added an __init__.py file to the openmc/data/material_libraries folder that helps the setup.py realize that these xml files should be copied

I think this is a deprecated approach. The more pyproject way I think would be through MANIFEST.in

@MicahGale
Copy link
Contributor

I don’t quite see how not including widely used libraries within OpenMC directly helps with QA exactly and to me it doesn’t make sense for something like the PNNL compendium to exist within every users custom libraries. Since properly including elements in material specifications does require significant refactoring, maybe it is worth implementing a temporary way of including user libraries, but im not sure.

If an organizational user doesn't want to use the libraries due to QA, they can just exclude that from the scope of their QA program.

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

Successfully merging this pull request may close these issues.

5 participants