-
Notifications
You must be signed in to change notification settings - Fork 5
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
Script to create pure fusion materials that user defines enrichment for #22
base: main
Are you sure you want to change the base?
Script to create pure fusion materials that user defines enrichment for #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good @FusionSandwich - I've made a few suggestions to make it more concise and readable.
Given all the formatting changes... it might be good to have some kind of CI test that makes sure we don't change the definition of each material in a meaningful way (first item in #18)
mat = Material(nucvec, density = density, metadata = {'citation' : citation}) | ||
|
||
def enrich( | ||
nucvec: Dict[int, float], old_key: int, new_values: Dict[int, float] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand the data structure properly (more info in the docstring would help), then old_key
is always an element ID and not an isotope/nuclide ID - is that right?
- let's choose a variable name that makes this clear - as well as better docstring
- What happens if
nucvec
is defined in terms of nuclides/isotopes already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes old_key is an element string. If nucvec is defined in terms of nuclides/isotopes the enrichment function won't work and it'll make the materials the same as if the dictionary didn't include mass_enrichment. In a future PR I plan on including logging to alert the user when this happens.
if old_key in nucvec: | ||
fract = nucvec.pop(old_key) | ||
for key, value in new_values.items(): | ||
nucvec[key] = value * fract | ||
else: | ||
found_key = None | ||
for key in list(nucvec.keys()): | ||
if nucname.id(old_key) == nucname.id(key): | ||
found_key = key | ||
break | ||
|
||
if found_key is not None: | ||
fract = nucvec.pop(found_key) | ||
for new_key, value in new_values.items(): | ||
nucvec[new_key] = value * fract # check if percent works also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking of a way to streamline so the key search is separate from the substitution so that each only needs to happen once:
nucvec_id_map = { nucname.id(key) : key for key in nucvec.keys() }
old_key_id = nucname.id(old_key)
if old_key_id in nucvec_id_map.keys():
nucvec_key = nucvec_id_map[old_key_id]
fract = nuvec.pop(nucvec_key)
for nuclide, enrich_frac in new_values.items():
nucvec[nuclide] = value * fract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a better method, fixed one part of it here. I tested it in the test file and it gives the same output.
nucvec_id_map = { nucname.id(key) : key for key in nucvec.keys() }
old_key_id = nucname.id(old_key)
if old_key_id in nucvec_id_map.keys():
nucvec_key = nucvec_id_map[old_key_id]
fract = nuvec.pop(nucvec_key)
for nuclide, enrich_frac in new_values.items():
nucvec[nuclide] = enrich_frac * fract
`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
old_key_id = nucname.id(old_key)
for key in list(nucvec.keys()):
if nucname.id(key) == old_key_id:
fract = nucvec.pop(key)
for nuclide, enrich_frac in new_values.items():
nucvec[nuclide] = enrich_frac * fract
`
Let me know if this looks better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This last version has an extra loop that seems unnecessary, but maybe I'm missing something about how this works. Isn't old_key
necessarily an element and therefore can only appear once in nucvec
?
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
This version of the createPurematlib.py incorporates new functions. mat_data is the same to demonstrate that its output matches exactly with main/createPurematlib.py. Another branch will contain a folder with test material libraries to help validate the new functions.
closes #19