search

terrapower / armi

An open-source nuclear reactor analysis automation framework that helps design teams increase efficiency and quality
https://terrapower.github.io/armi/
Apache License 2.0
231 stars 89 forks source link

Misleading Method Name (opinions wanted) #1753

Open albeanth opened 4 months ago

albeanth commented 4 months ago

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.

https://github.com/terrapower/armi/blob/f535add83f2a1322d455b93b5fc6c827731531e3/armi/reactor/composites.py#L2369-L2372

albeanth commented 4 months ago

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

https://github.com/terrapower/armi/blob/f535add83f2a1322d455b93b5fc6c827731531e3/armi/reactor/blocks.py#L1046-L1063

john-science commented 4 months ago

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 commented 4 months ago

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 commented 2 months ago

@albeanth examples are here:

https://github.com/terrapower/armi/blob/4b048efb3a39e65f37f7cb5ecc58a7883d2a71ba/armi/reactor/tests/test_blocks.py#L1300-L1306

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:

https://github.com/terrapower/armi/blob/9dc6760de7da3800cbe98e83066cf5f343e81a6a/armi/reactor/blocks.py#L1061-L1062

https://github.com/terrapower/armi/blob/9dc6760de7da3800cbe98e83066cf5f343e81a6a/armi/reactor/blocks.py#L1078

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 commented 2 weeks ago

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