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

Efficiency improvement #1042

Merged
merged 69 commits into from
Jan 16, 2023
Merged

Conversation

mgjarrett
Copy link
Contributor

@mgjarrett mgjarrett commented Dec 20, 2022

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

  • 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.

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.
@john-science john-science added the optimization related to measuring and speeding up the code or reducing memory label Dec 20, 2022
@mgjarrett mgjarrett linked an issue Dec 20, 2022 that may be closed by this pull request
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.
armi/reactor/blocks.py Outdated Show resolved Hide resolved
armi/reactor/blocks.py Outdated Show resolved Hide resolved
@keckler
Copy link
Member

keckler commented Jan 5, 2023

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:

  • I am using DIF3D/VARIANT P1P0 and MC2, so not the heaviest physics kernels
  • I am doing critical control rod iterations at every step
  • Anywhere between 1-4 depletion steps at each cycle, for nearly 50 cycles
  • No other physics
  • detailedAxialExpansion: True

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!!

Copy link
Member

@keckler keckler left a 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.

@keckler
Copy link
Member

keckler commented Jan 13, 2023

My test case worked well here. No complaints from me!

@keckler keckler linked an issue Jan 13, 2023 that may be closed by this pull request
Copy link
Member

@jakehader jakehader left a 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.

@mgjarrett mgjarrett merged commit 00507e1 into terrapower:main Jan 16, 2023
@mgjarrett mgjarrett mentioned this pull request Jan 18, 2023
7 tasks
drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Jan 23, 2023
…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",
Copy link
Member

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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization related to measuring and speeding up the code or reducing memory
Projects
None yet
4 participants