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

Process load update and new uniform mesher (beta) #735

Closed
wants to merge 30 commits into from

Conversation

albeanth
Copy link
Member

@albeanth albeanth commented Jun 24, 2022

Description

This addresses #705 and provides the beginnings of a new uniform mesher; though is not an ARMI converter like the original/current but is rather an ARMI changer.

The primary outcomes of this PR are:

  1. Blueprint assembly definitions are allowed to be axially disjoint in a core and have detailedAxialExpansion: False.
  2. The introduction of a new uniform mesher that aims to minimize (and in some cases remove) inter-block material smearing that is a result of the current uniform mesh converter
    .. warning: This procedure can cause numerical diffusion in some cases. For example,
    if a control rod tip block has a large coolant block below it, things like peak
    absorption rate can get lost into it. We recalculate some but not all
    reaction rates in the re-mapping process based on a flux remapping. To avoid this,
    finer meshes will help. Always perform mesh sensitivity studies to ensure appropriate
    convergence for your needs.
    • the new framework settings, primaryAssemblyToConserve and secondaryAssemblyToConserve provide options for users to provide strings that correspond to assembly Flags that group assemblies together and aim to best preserve their material interfaces. E.g., setting primaryAssemblyToConserve: "control" would give control assemblies highest priority in preserving their block bounds and would aim to minimize (or eliminate) inter-block material smearing (e.g., control into plenum).
    • Note, only assemblies of a single flag type can be grouped together.
    • The "primary" and "secondary" settings were included to to enable priorities for multiple assembly types.

Two other points:

  1. the new unit tests that come with this PR are particularly heavy... Future commits will be included to alleviate their cost.
  2. I included the new uniform mesher into the axial expansion changer here.
    # loop through again now that the reference is adjusted and adjust the non-fuel assemblies.
    refAssem = r.core.refAssem
    axMesh = refAssem.getAxialMesh()
    for a in r.core.getAssemblies(includeBolAssems=True):
    # See ARMI Ticket #112 for explanation of the commented out code
    a.setBlockMesh(
    axMesh
    ) # , conserveMassFlag=True, adjustList=adjustList)
    To improve ease of use, I changed the constructor for class AxialExpansionChanger and had to make a slew of associated adjustments.

Checklist

  • The code is understandable and maintainable to people beyond the author.
  • Tests have been added/updated to verify that the new or changed code works.
  • There is no commented out code in this PR.
  • The commit message follows good practices.
  • All docstrings are still up-to-date with these changes.

Summary:
- removing makeAxialSnapList and replacing with new uniform mesh changer
- now enables disjoint assemblies to pass processLoading with detailedAxialExpansion: False
Summary:
- to enable users to change which assembly groupings get mesh conservation priority, the variables "primaryAssemblyFlag" and "secondaryAssemblyFlag" were moved to be global settings.
- also updating runLog statements to make ARMI output cleaner. Users can increase the verbosity to debug to include more information and even set "printLikeBlockSmears = True" in getCoreWideUniformMesh if they _really_ want a lot of info. This could be made a runtime setting, but it seems less useful in a general sense, so leaving it as a source code change seems reasonable.
Summary:
- checkForSmearing had no self use, so it was made a function rather than a class method
Summary:
- things are waaaaay clearer and more organized if moved over into armi.reactor.converters. And sets things up a little better to eventually replace the old uniformMesher
- adding unit tests to ensure mass conservation at the block, assembly, and core levels. also has tests for exceptions to keep coverage up.
A few adjustments for other unit tests to pass:
1. applyCoreWideUniformMesh --> loop over getChildren() instead of getAssemblies to preserve assembly order in core. This is needed in test_reactors.py::HexReactorTests::test_isPickleable.
2. set self.core.p.axialMesh to self.core.finalAllAxialMeshPoints() --> this is needed to account for b.p.axMesh and the test in is needed in test_reactors.py::HexReactorTests::test_addMoreNodes.
3. insert 0.0 into uniMesher.unformMesh. Since core.p.axialMesh is set with findAllAxialMeshPoints(), the bottom mesh value of 0.0 is included now, so it needs to be added so this test passes.

TODO:
1. What to do if self.core.findAllAxialMeshPoints() returns a mesh with sub 1 cm meshes?
2. During process load, we don't _alwayss_ need to call the uniform mesher if detailedAxial is False and if self if a core. Really, it only needs to be run if the blueprints assemblies have disjoint axial meshes. This check should be added.
- This aligns with practice in the original uniform mesh converter. It also makes sense, actually. A users intent in setting b.p.axMesh != 1 is to allow the neutronics mesh to be finer than the geometric/armi mesh in the blueprints. However, the original intended neutronics mesh is lost once it passes through the uniform mesher. Once the uniform mesher changes the axial mesh, the intent of having a b.p.axMesh!= 1 is lost and may (well, likely) not make sense.
- for process load, if the assemblies are not disjoint, there is no reason to run the uniform mesher (it takes a little time and if it's not useful, then don't run it).
- removing this is opening up a rabbit hole
- Only affects _manageCoreMesh(r) which is called from axiallyExpandCoreThermal/Percent.
- for ease of use, adding primary/secondaryAssemblyToConserve flags to constructor
- with the removal of makeAxialSnapList from processLoad, it needs to be added here for the two tests in Class AssemblyInReactor_TestCase to pass.
- these tests very well may be the focus of another armi/github issue moving forward (TBD).
@albeanth albeanth marked this pull request as ready for review June 24, 2022 22:15
@albeanth
Copy link
Member Author

@john-science
Copy link
Member

@albeanth The tests are all borked. Just FYI.

@albeanth
Copy link
Member Author

albeanth commented Jun 24, 2022

Yeah... I'm seeing that and don't know why. They pass on my machine....

After running tox locally:

  py38: OK (326.52 seconds)
  lint: OK (490.03=setup[317.39]+cmd[172.64] seconds)
  cov: OK (297.69 seconds)
  congratulations :) (1114.34 seconds)

@john-science
Copy link
Member

Yeah... I'm seeing that and don't know why. They pass on my machine....

Two classic problems that can cause this:

  1. You didn't push all your code to GitHub. (check git status to see if this is true)
  2. Try running your tests exactly as GitHub does (python -m tox -e test)

changing so that strings are converted to flags through Flags.fromString
@albeanth
Copy link
Member Author

Adding some context for commit 54832a6.

The changes in this PR to reactor.py::Core::processLoading remove the call to makeAxialSnapList, which in turn, avoids a call to b.p.topIndex. So those params are never set and therefore the number of differences in the bookkeeping unit test is reduced.

@@ -154,6 +156,28 @@ def defineSettings() -> List[setting.Setting]:
"block. Used for axial mesh refinement.",
schema=vol.All(vol.Coerce(int), vol.Range(min=0, min_included=False)),
),
setting.Setting(
CONF_PRIMARY_ASSEMBLY_CONSERVE,
default="fuel",
Copy link
Member

Choose a reason for hiding this comment

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

I recommend adding a setter to this setting that transforms the inputs into valid flags so that we arent passing string inputs around.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I understand it this is exactly why the Flags system was created - to avoid passing around strings - so this totally makes sense.

Where should the setter go? Possibly on reactors.py::Core::setOptionsFromCs? Or is there somewhere else you have in mind?

for param in sourceBlk.getParamNames():
b.p[param] = sourceBlk.p[param]
for c in sourceBlk.getChildren():
b.add(c.__copy__())
Copy link
Member Author

Choose a reason for hiding this comment

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

@john-science Just wanted to bring this section to your attention since you have the open issue, #706, on deepcopies.

I made a concerted effort to avoid using deepcopy when creating a copy of an assembly -> block -> component, but gave in with components. I decided to use the Component::__copy__ method with the hope that when #706 is resolved, it too will be resolved. Anyway, just figured I'd get your eyes on this section in particular.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for keeping it in mind.

My concern is that "fixing" that will break a dozen major plugins, so it won't be possible for months.

Fingers crossed I'm just paranoid. We'll find out!

@john-science
Copy link
Member

This PR temporarily in a holding cycle while we fix the affects on downstream projects.

Soooooon!

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.

4 participants