-
Notifications
You must be signed in to change notification settings - Fork 91
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
Efficiency improvement #1042
Efficiency improvement #1042
Conversation
GammaUniformMeshConverter now inherits directly from UniformMeshGeometryConverter instead of the NeutronicsMeshConverter. This allows GammaUniformMeshConverter to skip the calcReactionRates step, which isn't necessary for gamma transport.
Use Block.createBlankCopy() method to copy the basic features of a block without using deepcopy. Still needs to use a deepcopy of the components, but we could replace that with a custom blank copy function for components.
When running createBlankCopy(), the new block will return an empty set for block.getNuclides() unless the number densities are explicitly set. Setting number density to 0.0 for each nuclide in self.getNuclides() ensures the same nuclides will be present in the numberDensities of the new block.
_calculateReactionRates operates on the whole destination assembly, but it was incorrectly positioned inside of the loop over blocks within the assembly, creating an unnecessary double-nested loop over blocks within an assembly. This is a significant factor slowing down the uniform mesh converter.
Just putting this here to help back up this PR. I've been testing locally with the version at the following hash: 81f1a81 This appears to speed up my calculation a lot. A calculation which previously took nearly 5 days is on track now to take only about 14 hrs. A little bit of information about that run to help put that in context:
This run was previously spending a lot of time doing uniform mesh conversions. That time has been reduced substantially, so the overall calculation time is now reduced by a factor of ~8-10. Awesome!! |
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.
The changes look good to me. I am running a test case and will comment again with the results of that.
My test case worked well here. No complaints from me! |
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.
Looks like this is functional and I propose to merge as soon as possible if @keckler and @john-science agree.
…30/assembly-1d-parent-class * terrapower/main: Adding measure of convergence for tight coupling (terrapower#1033) Fixing ruamel.yaml deprecation warning (terrapower#1109) Fix Issues with Peak Params (terrapower#1108) Adding a helper method to validate versions in settings files (terrapower#1102) Using fuelCycle settings constants (terrapower#1107) Using mockRunLogs in a better way (terrapower#1106) Using module-level neutronics settings constants (terrapower#1104) Cleanup umc (terrapower#1099) Fixing broken doc link for mpi4py (terrapower#1105) Improving logic for getNumPins (terrapower#1098) fix `code-block` sphinx errors (terrapower#1091) Defer setting material number in MCNP material card (terrapower#1086) Efficiency improvement for uniform mesh converter and general operations (terrapower#1042) Updating python version listed in README (terrapower#1076) Removing unused code from `calcReactionRates()` (terrapower#1084)
|
||
hexComponent = Hexagon( | ||
"homogenizedHex", | ||
"_Mixture", |
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.
@mgjarrett Can you help me understand why this new _Mixture
material is needed here? I see that it is just a subclass of Material
with no differences. Couldn't we have just built a Hexagon with Material
instead?
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.
Trying to reach back in the memory bank for what I might have been thinking...
I probably just wanted the material to have the name "Mixture" to make it clear that it's a mixture of materials.
I can't remember whether I tried just using "Material"
or whether I encountered any functional issues with it. Trying it now, it seems like unit tests do pass at least.
Description
The uniform mesh converter can take a very long time. Profiling the converter identified a couple of bottlenecks that turned out to be relatively low-hanging fruit. This is partially documented in #991 and #1039 .
These updates reduced uniform mesh conversion time by 80% by avoiding the
copy.deepcopy(block)
. This reduces the amount of detail stored on the uniform mesh reactor, but it is still sufficient for the physics that need to be performed on the uniform mesh reactor.These updates also reduced operator initialization time by about 40% when thermal expansion is enabled.
Checklist
doc/release/0.X.rst
) are up-to-date with any bug fixes or new features.doc
folder.setup.py
.