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
214 stars 82 forks source link

Cleaning out the overly-specific methods in ArmiObject #1667

Closed john-science closed 2 months ago

john-science commented 3 months ago

What is the change?

Removing extraneous ArmiOjbect methods:

Why is the change being made?

There are several methods in ArmiObject (and Composite) that seem a little too specific to be included everywhere in the ARMI object tree. And after doing some research, some of them are entirely unused anyway.

Close #1375


Checklist

john-science commented 3 months ago

Bump

john-science commented 3 months ago

@albeanth

Moving ArmiObject.getBoronMassEnrich() to Block Moving ArmiObject.getPuMoles() to Block Moving ArmiObject.getUraniumMassEnrich() to Block

I could see utility in having this at the composite level....

If it helps, I tested this PR downstream in all repos, including benchmark repos, it passes. So that is firm confirmation that these methods are NOT used at the Composite level, if that helps.

Cards on the table...

None of these methods should have been added to the API in the first place. That was the cause of Jake/Mark's complaint. They are fair too specific. An entire method for just one nuclide? If we have one for "boron mass' why not "boron moles" and "uranium mass" and "uranium moles" and OMG that paradigm leads us to hundreds and hundreds of nearly identical stupid methods.

Jake and Mark make a compelling argument: (1) remove any of these too-specific methods we can, and (2) reduce the surface area of the ones we can't remove.

Moving these methods from ArmiObject / Composite to something less specific means these methods aren't usable everywhere. (Which, they aren't now.) And that's a good thing. Because we want to limit how much these methods are used.

albeanth commented 3 months ago

They are fair too specific. An entire method for just one nuclide? If we have one for "boron mass' why not "boron moles" and "uranium mass" and "uranium moles" and OMG that paradigm leads us to hundreds and hundreds of nearly identical stupid methods.

Oh yeah, totally agree here. I wonder if they could be generalized? Hmmm, likely not in a useful/elegant way. The underlying functionality of these is retained, so any downstream user could recreate them as needed.

Ok, I will retract. Remove!

john-science commented 3 months ago

Oh yeah, totally agree here. I wonder if they could be generalized?

Yeah, I was thinking something like:

# class ArmiObject
    def getNuclide(self, nuclide, units="mass"):
        """Note: units needs to be in ("mass", "frac", "moles")"""

Or, you know, something like that. Some sort of more general method that replaces all this stuff.

Also, I note that we already have ArmiObject.getNumberDensity(), which takes a nuclide name and does most of the heavy lifting all these other methods do; it's just a difference of units.

just-thinking-aloud

albeanth commented 3 months ago

Oh yeah, totally agree here. I wonder if they could be generalized?

Yeah, I was thinking something like:

# class ArmiObject
    def getNuclide(self, nuclide, units="mass"):
        """Note: units needs to be in ("mass", "frac", "moles")"""

Or, you know, something like that. Some sort of more general method that replaces all this stuff.

Also, I note that we already have ArmiObject.getNumberDensity(), which takes a nuclide name and does most of the heavy lifting all these other methods do; it's just a difference of units.

just-thinking-aloud

You could always do a poll in the discussion on what would be useful!