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

Update key for enrichment material modification in Lithium material class #546

Merged
merged 14 commits into from
Jan 26, 2022
Merged

Update key for enrichment material modification in Lithium material class #546

merged 14 commits into from
Jan 26, 2022

Conversation

keckler
Copy link
Member

@keckler keckler commented Jan 24, 2022

Description

The material modification keyword for enrichment in the Lithium material class is ambiguous to the user as to which isotope is being enriched. This is particularly confusing for the case of lithium, because 'enriched lithium' means different things to different people. People who want to use lithium as an absorber typically think 'enriched' refers to Li6, while those who use lithium in coolant materials think 'enriched' refers to Li7.

In addition, the keyword is just inconsistent with all the other material classes which employ enrichment material modifications. In all other existing cases, the isotope is specified in the key.

This PR updates the key for the enrichment material modification in the Lithium class to avoid the ambiguity and be consistent with the other material classes.

Tests have been added.


Checklist

  • Tests have been added/updated to verify that the new or changed code works.
  • Docstrings were included and updated as necessary
  • The code is understandable and maintainable to people beyond the author
  • There is no commented out code in this PR.

If user exposed functionality was added/changed:

  • Documentation added/updated in the doc folder.
  • New or updated dependencies have been added to setup.py.

@john-science
Copy link
Member

I have the same feeling about this as I did about the Thorium Oxide PR: it seems good, I wonder why this wasn't necessary before.

I suppose the main concern is: do we think this will break anyone's downstream models?

Perhaps we can talk about that over some sort of a call today?

Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

Nice.

Also, thanks again for the unit test coverage of the new code.

@john-science john-science merged commit fe4e1de into terrapower:master Jan 26, 2022
@keckler keckler deleted the LI6_wt_frac-v2 branch February 4, 2022 18:59
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
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

Successfully merging this pull request may close these issues.

None yet

2 participants