Skip to content
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

Cleaning out the overly-specific methods in ArmiObject #1667

Merged
merged 14 commits into from
Apr 8, 2024

Conversation

john-science
Copy link
Member

@john-science john-science commented Mar 21, 2024

What is the change?

Removing extraneous ArmiOjbect methods:

  • Moving ArmiObject.getBoronMassEnrich() to Block
  • Moving ArmiObject.getPuMoles() to Block
  • Moving ArmiObject.getUraniumMassEnrich() to Block
  • Removing ArmiObject.getMaxUraniumMassEnrich.()
  • Removing ArmiObject.getMaxVolume() & Block.getMaxVolume()
  • Removing ArmiObject.getPuFrac()
  • Removing ArmiObject.getPuMass()
  • Removing ArmiObject.getPuN()
  • Removing ArmiObject.getZrFrac()
  • Removing ArmiObject.printDensities()
  • Moving Composite.isOnWhichSymmetryLine() to Assembly
  • Removing Block.isOnWhichSymmetryLine()

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

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

@john-science john-science added the cleanup Code/comment cleanup: Low Priority label Mar 21, 2024
@john-science john-science requested review from onufer and albeanth and removed request for jakehader March 26, 2024 16:25
@john-science
Copy link
Member Author

Bump

Copy link
Member

@albeanth albeanth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm.

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. I don't think it's unreasonable to have the ability to query these things at the Assembly, Core, or Component level (specifically on the Component as well, for flexibility with future applications)

armi/reactor/components/__init__.py Outdated Show resolved Hide resolved
armi/reactor/components/__init__.py Outdated Show resolved Hide resolved
armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved
armi/reactor/tests/test_blocks.py Outdated Show resolved Hide resolved
@john-science
Copy link
Member Author

@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
Copy link
Member

albeanth commented Apr 2, 2024

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
Copy link
Member Author

john-science commented Apr 2, 2024

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
Copy link
Member

albeanth commented Apr 2, 2024

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!

Co-authored-by: Tony Alberti <aalberti@terrapower.com>
@john-science john-science requested review from albeanth and removed request for onufer April 5, 2024 16:25
@john-science john-science merged commit 6b34896 into main Apr 8, 2024
21 checks passed
@john-science john-science deleted the test_armi_object_changes branch April 8, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audit Composite/ArmiObject methods and their applicability to all objects
2 participants