Should component volume consider symmetryFactor?

[albeanth]

In #1954, we are starting to store more information at the component level. One of those things is the decay power (powerDecay). While looking at some downstream test results with these changes, we noticed that the center-most assembly block-wise decay power was not properly accounting for the symmetry factor. After some discussion, we found that it was due to the fact that we were calculating the block-wise decay power from component-level decay power; and the latter was getting calculated without consideration to the symmetry factor.

E.g., we typically calculate block-wise power by doing something like:

b.decayPower = b.decayPowerDensity * b.getVolume()

where b.getVolume() accounts for the symmetry factor. However, a block-wise decay power derived from component-level decay power looks something like:

for c in b:
    c.decayPower = c.decayPowerDensity * c.getVolume()

b.decayPower = sum(c.decayPower for c in b)

where c.getVolume() does not respect the symmetry factor.

So all this said, should c.getVolume() respect the symmetry factor? This could look something like:

[john-science]

@albeanth "decayPower" is not a parameter in ARMI. That sounds like some MCNP value.

should c.getVolume() respect the symmetry factor?

For your situation, it seems like a clear "yes". But Component.computeVolume() is used in just SO MANY place, so.... can I say for sure, right now, that making the suggested change is correct everywhere? No, I can't say that.

But:

1. We can just test your idea, and see if a million things break.
2. EVEN IF a bunch of other things break, we could add a parameter to that method:

def computeVolume(self, respectSymmetry=False):

So, it seems fixable either way.

[albeanth]

"decayPower" is not a parameter in ARMI.

It was until #1954. Either way, that's not the important part here.

I agree, component.getVolume() is used an a gazillion places - and is why I wanted to open this Issue first and see what others thoughts were. Just changing it will likely have quite the ripple effect.

We could do the optional param, that would be easy. But if it is indeed a bug, then maybe the optional param isn't the "right" option and really it should just be hardcoded in. That's the part I don't know and wanted to get the opinion of others on.

[jakehader]

"decayPower" is not a parameter in ARMI.

It was until #1954. Either way, that's not the important part here.

I agree, component.getVolume() is used an a gazillion places - and is why I wanted to open this Issue first and see what others thoughts were. Just changing it will likely have quite the ripple effect.

We could do the optional param, that would be easy. But if it is indeed a bug, then maybe the optional param isn't the "right" option and really it should just be hardcoded in. That's the part I don't know and wanted to get the opinion of others on.

I'm not sure I agree with a keyword argument here. To me it seems like the multiplicity should be scaled back with the symmetry so that this would be captured on self.getArea(). This seems like it could be a bug in the pin level parameters if we don't account for this in the spatial grid and number of components in the block.

[albeanth]

Was adding a test to Block.completeInitialLoading and noticed that the component-wise heavy metal mass here calls out the symmetry factor. So it seems as though someone at some point thought about this.

armi/armi/reactor/blocks.py

Line 797 in 864106e

hmMass = child.getHMMass() * sf

If the component volume considered the symmetry factor then the sf factor here wouldn't be needed.

[albeanth]

#1990 (comment)

cross referencing here for posterity

[john-science]

Before we can get started on this ticket, I guess what I want is some sort of high level view of all the places this change will impact.

I have (perhaps temporarily) added "complex" as a label to this ticket because my guess is that if we follow Tony's suggestion above and change the default behavior here, this will have far-reaching and hard-to-catch changes in ARMI and downstream.

Of course, if we can itemize all the places this affects and the list is small... great! That's not a problem. I would love to be wrong.

[albeanth]

Was discussing this issue with a teammate yesterday and discovered that there may be other symmetry factor related issues. I think this work could consider expanding to a general audit of the use of symmetry factor throughout ARMI.

See here for some awkward use of the symmetry factor (it gets awkward to keep things consistent).

armi/armi/reactor/blocks.py

Lines 796 to 811 in 01a5adf

	# multiplying by sf ends up cancelling out the symmetry factor used in
	# Component.getMass(). So massHmBOL does not respect the symmetry factor.
	hmMass = child.getHMMass() * sf
	massHmBOL += hmMass
	# Components have the following parameters but not every composite will
	# massHmBOL, molesHmBOL, puFrac
	if isinstance(child, components.Component):
	child.p.massHmBOL = hmMass
	# to stay consistent with massHmBOL, molesHmBOL and puFrac should be
	# independent of sf. As such, the need to be scaled by 1/sf.
	child.p.molesHmBOL = child.getHMMoles() / sf
	child.p.puFrac = (
	self.getPuMoles() / sf / child.p.molesHmBOL
	if child.p.molesHmBOL > 0.0
	else 0.0
	)

[keckler]

The number of issues that this central assembly has caused in 1/3 core models is just laughable. Here is at least one, and I know of multiple other internal instances of similar troubles: #742

At this point I am cautious to believe hardly anything about the central assembly without doing a lot of manual verification.

[bsculac]

I've coincidentally been working on something similar due to a downstream bug, see #2017. I think adding a symmetry scaling check to moves makes the most obvious sense. Core.add uses Assembly.moveTo for placing a newly created assembly in the core, so this accounts for new assembly addition.

Conceptually, I think we'd want to have the assembly parameters populated before placement in the core, then the placement and movement would account for adjustments due to the symmetry factor.

[bsculac]

I was just talking with @albeanth about this and he had the idea of baking the symmetry factor into the parameter getters. Noting it here for future reference.

[albeanth]

I was just talking with @albeanth about this and he had the idea of baking the symmetry factor into the parameter getters. Noting it here for future reference.

This could a slow operation though. Not sure. The idea was to try and prevent devs from even using the symmetry factor at all. It's a finnicky little thing (at least currently) and looks to be error prone. Exposing it in one place would help this, I think.