-
Notifications
You must be signed in to change notification settings - Fork 87
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
Misleading Method Name (opinions wanted) #1753
Comments
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). Lines 1046 to 1063 in f535add
|
I think the name
In 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. |
It's just weird to me. If we have a typical pinned fuel block consisting of the following:
I, personally, would expect 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 Also, as an aside: |
@albeanth examples are here: armi/armi/reactor/tests/test_blocks.py Lines 1300 to 1306 in 4b048ef
Here I see two entirely separate concerns:
My thoughts:
Lines 1061 to 1062 in 9dc6760
Line 1078 in 9dc6760
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? |
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 begetNumPins
.armi/armi/reactor/composites.py
Lines 2369 to 2372 in f535add
The text was updated successfully, but these errors were encountered: