Misleading Method Name (opinions wanted)

[albeanth]

As I see it, in general, an ARMI Component != a pin (yes you could define Component as a single pin, but in my experience this is not generally the case). That said, I think this method name is misleading. Instead of getNumComponents I would advocate that this be getNumPins.

armi/armi/reactor/composites.py

Lines 2369 to 2372 in f535add

	def getNumComponents(self, typeSpec: TypeSpec, exact=False):
	"""
	Get the number of components that have these flags, taking into account multiplicity. Useful
	for getting nPins even when there are pin detailed cases.

[albeanth]

Just found this method in blocks.py as well which is likely a more useful method to retrieve the number of pins. If one wanted to maintain functionality on the Assembly and Core levels, I would suggest that specific methods be written on those classes to loop over the appropriate composites (or maybe this method on block.py is generalized to the Composite class, I'm not sure).

armi/armi/reactor/blocks.py

Lines 1046 to 1063 in f535add

	def getNumPins(self):
	"""Return the number of pins in this block.
	.. impl:: Get the number of pins in a block.
	:id: I_ARMI_BLOCK_NPINS
	:implements: R_ARMI_BLOCK_NPINS
	Uses some simple criteria to infer the number of pins in the block.
	For every flag in the module list :py:data:`~armi.reactor.blocks.PIN_COMPONENTS`,
	loop over all components of that type in the block. If the component
	is an instance of :py:class:`~armi.reactor.components.basicShapes.Circle`,
	add its multiplicity to a list, and sum that list over all components
	with each given flag.
	After looping over all possibilities, return the maximum value returned
	from the process above, or if no compatible components were found,
	return zero.

[john-science]

I think the name getNumComponents() is sufficiently generic, and accurate to it's goal.

ArmiObject.getNumComponts() doesn't just get "pins". As the docstring says, it finds all the sub-components of the given type.

In blocks.py, we use this to get the number of fuel sub-components, so we can calculate the area of the cladding.

Looking downstream, this is used to get number of pins, sure, but it is also used to find the number of clad or duct items in an area.

[albeanth]

It's just weird to me. If we have a typical pinned fuel block consisting of the following:

1. fuel, mult = 169
2. clad, mult = 169
3. duct, mult = 1
4. intercoolant, mult = 1
5. coolant, mult = 1

I, personally, would expect getNumComponents(Flags.FUEL) to return 1. There's only 1 fuel component... Now if that method was called something like getTotalMultForComponentsOfType(Flags.FUEL) then that would clearly return 169 and would be way clearer, imo.

There's also a scenario that doesn't look to be tested (at least not in test_block.py). Consider the case in which a block has multiple types of pins (e.g., a mix of fuel and reflector pins). In this case it looks like getNumComponents has better/more predictable behavior than getNumPins - so that's fun lol. Check out this feature branch I made to play with this. Let me know what you think?
https://github.com/terrapower/armi/tree/playWithPinNumbers

Also, as an aside:
getNumComponents is tested in test_blocks.py and looks to only be used on blocks in ARMI and one major in-house project. We could maybe/probably make getNumComponents a Block method.

[john-science]

@albeanth examples are here:

armi/armi/reactor/tests/test_blocks.py

Lines 1300 to 1306 in 4b048ef

	print(testBlock.getNumComponents(Flags.FUEL)) # is 127 (TA thinks should be 1)
	print(testBlock.getNumComponents([Flags.FUEL, Flags.PIN])) # is 169 (TA thinks should be 2)
	print(testBlock.getNumPins()) # is 127 (TA thinks this should be 169...)
	## Also, getNumPins() should allow for a list of flags so users could do the following
	## testBlock.getNumPins(Flags.FUEL) # returns 127
	## testBlock.getNumPins([Flags.FUEL, Flags.PIN]) # returns 169
	## or for a default behavior, testBlock.getNumPins() # returns all pins regardless of flags, so this would be 169

Here I see two entirely separate concerns:

1. The getNumComponents() returns values respecting multiplicity, but Tony did not expect that or finds that counterintuitive. (Tony mostly uses this method to replace getNumPins().)
2. The getNumPins() does not work correct, or as Tony expected.

My thoughts:

1. I actually think it's great that getNumComponents() respects multiplicity. We do in many, many places in ARMI, and if we didn't, multiplicity would be meaningless. The ARMI Reactor model is designed with multiplicity in mind at nearly all levels.I could happily call this out even more explicitly in the docstring, if you like.
2. I think it is possible there is a bug here. Or at very least, a feature I don't understand. When counting "pins" in a block, it doesn't sum all the pins in each component, but picks the largest NUMBER of pins in a component:

armi/armi/reactor/blocks.py

Lines 1061 to 1062 in 9dc6760

	After looping over all possibilities, return the maximum value returned
	from the process above, or if no compatible components were found,

armi/armi/reactor/blocks.py

Line 1078 in 9dc6760

return 0 if not nPins else max(nPins)

This is called out explicitly in the requirement, but I can't say I fully understand WHY. The only way this makes sense is if Tony's example above is erroneous, because we would not normally have two different components with different pin types in the same block. But if THAT were true... why would we need the "max" in this method anyway?

[albeanth]

why would we need the "max" in this method anyway?

No idea.

Coming across this again with pin-related level work... It's a bit frustrating, but maybe it's just my own problem! I'll just pretend the blocks.getNumPins() doesn't exist.... 🤷