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

Give Interface.updatePhysicsCouplingControl() a better name #1779

Open
keckler opened this issue Jul 15, 2024 · 1 comment
Open

Give Interface.updatePhysicsCouplingControl() a better name #1779

keckler opened this issue Jul 15, 2024 · 1 comment
Labels
feature request Smaller user request

Comments

@keckler
Copy link
Member

keckler commented Jul 15, 2024

The Interface class has a placeholder for a method called updatePhysicsCouplingControl():

armi/armi/interfaces.py

Lines 648 to 650 in d0092e8

def updatePhysicsCouplingControl(self):
"""Adjusts physics coupling settings depending on current state of run."""
pass

Typically the placeholder methods on the Interface class are put there as hooks that are called while looping over the interface stack, and so the Interface class just implements a pass and child interfaces can implement their own. But looking through the ARMI codebase, it doesn't appear that this method is ever called. So what is it?

It turns out that this method is intended to be used in a very specific way which is associated with our internal equilibrium plugin. We have an equilibrium interface, and when it is being run we don't always want all of the interfaces to be executed in the normal way. So when the equilibrium interface runs, it calls to this updatePhysicsCouplingControl() method on all of the other interfaces, each of which has the opportunity to modify its behavior for the equilibrium calculation. This is a pretty weird design choice for quite a few reasons.

But aside from the strange design choice, this method name and docstring do not give any indication of what this method is actually used for. I have the following suggestions, in order of increasing complexity:

  1. At the least, the docstring on the parent class should be updated to indicate that the method is called from within the equilibrium plugin.
  2. In addition to updating the docstring, the method name should be updated to make it obvious what the method is intended to be used for. Maybe something like updatePhysicsCouplingControlForEquilibrium()?
  3. It would make a lot of sense to move the calling of the method from within our internal equilibrium interface onto the operator in ARMI. Wrap it in something like if self.cs["runType"] == operators.RunTypes.EQUILIBRIUM
@john-science john-science added the feature request Smaller user request label Jul 15, 2024
@john-science
Copy link
Member

The method updatePhysicsCouplingControl() is used in subclasses downstream, so that should be dealt with for option (2) above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

No branches or pull requests

2 participants