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

Add setting for some assemblies to skip axial expansion #1235

Merged
merged 8 commits into from
Apr 25, 2023

Conversation

albeanth
Copy link
Member

@albeanth albeanth commented Apr 5, 2023

Description

It may be desired to have some assemblies skip axial expansion routines. This PR adds a setting, assemFlagsToSkipAxialExpansion to accomplish this.

Users can provide assembly flags in a list in the input setting. If an assembly contains a flag that matches those listed, it is skipped for axial expansion.


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.

- to account for new setting to skip axial expansion of assemblies
- also sneaks in replacing some instances of cs[<string>] with cs[CONF_XYZ]
@albeanth albeanth marked this pull request as ready for review April 6, 2023 19:17
@albeanth albeanth requested a review from keckler April 6, 2023 19:17
@albeanth
Copy link
Member Author

albeanth commented Apr 6, 2023

@onufer FYI

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.

A few comments, one of which (with the skipping of the control assems) I would like to get answered before approval.

If true assemblies will be forced to conform to the reference mesh after expansion
If False, assemblies will be forced to conform to the reference mesh after expansion
assemsToSkip: List[str]
list of strings to be converted to flags to indicate which assemblies to skip axial expansion
Copy link
Member

Choose a reason for hiding this comment

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

It would seem better to me to just pass the flags directly. That way someone who has the flags doesn't need to de-convert them into strings in order to use this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed 32ce614

armi/reactor/converters/axialExpansionChanger.py Outdated Show resolved Hide resolved
Comment on lines 352 to 354
self.assemblies.values(),
cs[CONF_DETAILED_AXIAL_EXPANSION],
cs[CONF_ASSEM_FLAGS_SKIP_AXIAL_EXP],
Copy link
Member

Choose a reason for hiding this comment

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

This is not incredibly important, but...

With this change, you change the API of expandCoddDimsToHot(). And in fact you make the new parameter mandatory, so the change is also not backwards compatible.

My inclination would be to avoid changing the signature of expandColdDimsToHot(), and instead to just whittle down the list returned from self.assemblies.values() right here when you pass it in. Something like the following:

assemsToExpand = [a for a in list(self.assemblies.values()) if not a.hasFlags(cs[CONF_ASSEM_FLAGS_SKIP_AXIAL_EXP])
axialExpansionChanger.expandColdDimsToHot(assemsToExpand, cs[CONF_DETAILED_AXIAL_EXPANSION])

Just a suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed. 32ce614

@albeanth albeanth requested a review from keckler April 6, 2023 22:50
Comment on lines 86 to 87
if a.hasFlags(Flags.CONTROL) or any(a.hasFlags(aFlags) for aFlags in aToSkip):
continue

Choose a reason for hiding this comment

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

Assuming it's okay to require exact type IDs here, e.g., "duct" wouldn't match "innerduct", you could just do the following:

Suggested change
if a.hasFlags(Flags.CONTROL) or any(a.hasFlags(aFlags) for aFlags in aToSkip):
continue
if a.hasFlags(Flags.CONTROL) or a.hasFlags(aToSkip, exact=True):
continue

Copy link
Member Author

@albeanth albeanth Apr 7, 2023

Choose a reason for hiding this comment

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

My concern with using exact if you have the following:

import armi
armi.configure()
from armi.reactor.blocks import HexBlock

b0 = HexBlock("fuel")
b0.setType("fuel")
b1 = HexBlock("outer fuel")
b1.setType("outer fuel")

# Part 1: Use exact to get false negative
print(b1.hasFlags(typeID=[b0.p.flags]))
print(b1.hasFlags(typeID=[b0.p.flags], exact = True))

# Part 2: Gives false negative always
print(b0.hasFlags(typeID=[b1.p.flags]))
print(b0.hasFlags(typeID=[b1.p.flags], exact = True))

# Part 3: The correction for part 2
print(any(b0.hasFlags(f) for f in b1.p.flags))
print(any(b0.hasFlags(f, exact=True) for f in b1.p.flags))

which prints

True
False
False
False
True
True

So using the any approach is a bit more robust, I think. And since in that case we're checking flags one by one, the use of exact is irrelevant. This same approach is also used in Core::processLoading here,

armi/armi/reactor/reactors.py

Lines 2272 to 2276 in c8a40c7

uniformAssems = [
a
for a in self.getAssemblies()
if not any(a.hasFlags(f) for f in nonUniformAssems)
]

@john-science
Copy link
Member

@albeanth This is sitting in a queue for a downstream project. Merging one day soon, hopefully.

@albeanth albeanth marked this pull request as draft April 19, 2023 22:27
@albeanth albeanth marked this pull request as ready for review April 25, 2023 15:47
@albeanth albeanth merged commit 342d2c9 into terrapower:main Apr 25, 2023
9 checks passed
@albeanth albeanth deleted the addSettingSkipAxExp branch April 25, 2023 16:09
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.

None yet

4 participants