SOLVED: Why is None supplied for Helix dimensions defaults?

[keckler]

Typically when you construct a component, ARMI gives a good error message if you forget to supply a required dimension of the component:

[err ] Potentially invalid kwargs {'mergeWith': '', 'isotopics': '', 'name': 'clad', 'material': <Material: HT9>, 'Tinput': 25.0, 'Thot': 450.0, 'mult': 271, 'id': 0.6962} for <class 'armi.reactor.components.basicShapes.Circle'> of shape circ
le. Check input.
TypeError: __init__() missing 1 required positional argument: 'od'

But I noticed that when you construct a Helix component, if you accidentally forget to supply one of the dimensions of the component then the code gives a different, less useful error:

TypeError: unsupported operand type(s) for +: 'NoneType' and 'ScalarFloat'

I think the reason that the Helix doesn't give the good error message is because the __init__ of the Helix class supplies default values of None for the required dimensions to the component constructor. Is there some good reason for supplying these defaults? Otherwise, I'm thinking we should take those away to improve the user experience.

[john-science]

Well, if we compare Circle.__init__():

class Circle(ShapedComponent):
    def __init__(
        self,
        name,
        material,
        Tinput,
        Thot,
        od,
        id=0.0,
        mult=1.0,
        modArea=None,
        isotopics=None,
        mergeWith=None,
        components=None,
    ):

With Helix.__init__():

class Helix(ShapedComponent):
    def __init__(
        self,
        name,
        material,
        Tinput,
        Thot,
        od=None,
        id=0.0,
        mult=None,
        axialPitch=None,
        helixDiameter=None,
        modArea=None,
        isotopics=None,
        mergeWith=None,
        components=None,
    ):

There are definitely some different initializers there. First, Helix has decided od isn't mandatory, which I find suspicious. Next, Helix gives a None default to axialPitch, mult, and helixDiamter, and I find those a bit suspicious, too. Especially mult.

So, there are two approaches I can envision here:

1. We provide Helix with some different defaults.
2. We try to track down the real problem: why Helix doesn't fail sooner and throw a more sensible error.

[keckler]

Well I think that Helix would fail better if it had the appropriate initializers. So I think that if we just take away the None defaults on od, axialPitch, and helixDiameter, then the error messages would be improved.

And yes, I agree as well that mult should default to 1.

[john-science]

While I like the idea of improving the defaults to the Helix class, I cannot actually reproduce your bug:

>>> from armi.reactor.components.complexShapes import Helix
>>> h = Helix("test", "HT9", 322, 555)
>>>
>>> h = Helix("test", "HT9", 322)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() missing 1 required positional argument: 'Thot'
>>>
>>> h = Helix("test", "HT9")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() missing 2 required positional arguments: 'Tinput' and 'Thot'
>>>
>>> h = Helix("test", "HT9", 322, id=0.1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() missing 1 required positional argument: 'Thot'

The error messages look descriptive to me. How were you able to get that error?

TypeError: unsupported operand type(s) for +: 'NoneType' and 'ScalarFloat'

[keckler]

Weird. I'll put an example up later today.

[keckler]

So the issue is not with the initialization of the component shape itself, but rather when the component is used in the construction of an assembly. It will be hard to recreate the issue with just a small code snippet like what you posted above, but I attached an ARMI case here that spits out the unhelpful error message.

And I can confirm that the error message changes to the more-helpful version if you remove the None initializers from the __init__.

[john-science]

Okay, my current plan for the Helix class is:

1. Change the default for mult to be 1.0.
2. Remove the defaults for: od, helixDiameter, and axialPitch.

My reasoning goes as follows:

1. That was a terrible default for mult, and no one was using it.
2. There is no way to define a helix without these extra numerical properties. So it seems impossible that anyone has used this class without them.

The only risk I can see is that changing the above involves a slight change to the order of the __init__() parameters, which might cause a hiccup somewhere downstream.

[john-science]

There is a PR open for this Discussion here: #558