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

TD_frac updates and consistency #1114

Closed
wants to merge 35 commits into from
Closed

Conversation

keckler
Copy link
Member

@keckler keckler commented Jan 23, 2023

Description

This closes #1112 by ensuring that the TD_frac material modification on UraniumOxide and MOX both actually change the density as they are supposed to.

This is implemented mainly via changes to the SimpleSolid class and by making setting a default value of 1.0 to the theoreticalDensityFrac material parameter.

Other than that, the other material classes that were originally using some form of a theoreticalDensityFrac variable were modified to be consistent in their usage of that variable. Those other material classes included ThoriumOxide and B4C.

This doesn't touch anything in the API, so users should not notice any changes except the fact that TD_frac is now correctly applied, so their results might change if they were using the framework oxide classes.

EDIT: This PR no longer makes any changes to B4C. A new ticket was opened to address that class.


Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
  • The release notes (location doc/release/0.X.rst) are up-to-date with any bug fixes or new features.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@keckler keckler requested a review from john-science January 23, 2023 19:43
@keckler keckler marked this pull request as draft January 23, 2023 19:51
@keckler
Copy link
Member Author

keckler commented Jan 23, 2023

I am testing our internal projects against this change now, and will update when I know that those work.

@keckler
Copy link
Member Author

keckler commented Jan 23, 2023

This appears to be working. I had to make one change, to the internal version of B4C. But that was easy to do, and now this should be ready to land.

@keckler keckler marked this pull request as ready for review January 23, 2023 22:45
@keckler
Copy link
Member Author

keckler commented Jan 24, 2023

I see something is failing... grrr

@keckler
Copy link
Member Author

keckler commented Jan 24, 2023

Okay @john-science , this is ready for review.

@keckler
Copy link
Member Author

keckler commented Jan 24, 2023

And now I need to change this to account for #1062

@john-science john-science added the enhancement New feature or request label Jan 24, 2023
@keckler
Copy link
Member Author

keckler commented Jan 24, 2023

I have no idea why these tests are failing. They all pass locally, and looking at the ones that claim to be failing, they seem pretty random...

@keckler
Copy link
Member Author

keckler commented Jan 25, 2023

@john-science Have you seen this before? The tests passed with Python3.9 but not 3.10...

Copy link
Member

@ntouran ntouran left a comment

Choose a reason for hiding this comment

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

would be good to update the user docs to explain this in the materials docs.

armi/materials/material.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TD_frac for UraniumOxide and MOX doesn't do anything
3 participants