-
Notifications
You must be signed in to change notification settings - Fork 89
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
Strawman fix for the custom isotopics for testing. #1822
base: main
Are you sure you want to change the base?
Conversation
@jakehader Just a reminder of the ARMI PR policy: armi/doc/developer/tooling.rst Lines 45 to 49 in bd0b43a
We are all extremely busy, and if a PR gets opened before it is ready I will get an email:
I love the help, and the interesting in fixing the feature. But in the future, please wait until the feature is ready for final review before opening a PR. |
print(comp.density()) | ||
print(f) | ||
print(comp.inputTemperatureInC) | ||
print(comp.temperatureInC) | ||
scaledDensity = comp.density() / f | ||
densityRatio = density / scaledDensity | ||
comp.changeNDensByFactor(densityRatio) | ||
print(comp.density()) |
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.
Reminder to remove print statements.
Thanks for the reminder @john-science - I put the PR here but forgot to make it a draft. This was just intended to provide some ideas for how to resolve the ticket and to put something out there to be stress tested by @keckler and others. There are failing unit tests because I changed the construct arguments so that needs to be resolved still (probably a poor design to require case settings to build a component). Also, we will need to add more unit tests around custom Isotopics (same comment on the ticket). |
I ran some tests on my FFTF model to see if this branch works. A couple points:
So it seems like this change is half working. Somehow the initialized model is getting the densities very incorrect, though. |
Was this fixed elsewhere @keckler? |
Nope, still broken |
What is the change?
Why is the change being made?
Checklist
doc
folder.pyproject.toml
.