search

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
222 stars 87 forks source link

Accessing `CaseSettings` from children of `ArmiObject` or `Composite` #660

Closed albeanth closed 2 years ago

albeanth commented 2 years ago

The Issue

Accessing attributes of CaseSettings from within children of ArmiObject or Composite is not straightforward; specifically from within instances of Component, Block, and Assembly. As of now, when a Reactor object is built from the blueprints, a subset of CaseSettings get stored in https://github.com/terrapower/armi/blob/22399244a88e67fc09996981081f7d5aab7032b4/armi/reactor/reactors.py#L224-L232

A few ideas

  1. If in a component and need to access the new setting inputHeightsConsideredHot (being introducing in #657) , one could use c.parent.parent.parent._inputHeightsConsideredHot (if the variable self._inputHeightsConsideredHot was added to setOptionsFromCs).
  2. Using getAncestor. As an example, this returns the Core object from a block. https://github.com/terrapower/armi/blob/22399244a88e67fc09996981081f7d5aab7032b4/armi/reactor/blocks.py#L161-L166
  3. ...anything else?
albeanth commented 2 years ago
  1. Add a class method setOptionsFromCs(self,cs) to each class that needs specific CaseSettings attributes. Just like is done for Core. https://github.com/terrapower/armi/blob/22399244a88e67fc09996981081f7d5aab7032b4/armi/reactor/blueprints/reactorBlueprint.py#L143-L147

My hesitation is with the TODO statement. Makes me think it may not be the best idea...?

albeanth commented 2 years ago
  1. Following something along the ideas of the TODO statement in my previous comment... Could we subclass things? It may not be super conventional or feasible, but would provide ArmiObject and all its children direct access to all of the attributes within CaseSettings. image
john-science commented 2 years ago

@ntouran @albeanth

I am not sure I am in favor of this change. It just seems like... we've made it this long without needing this access. So it must be possible to do what we need without adding it now.

In particular, I believe this is entirely driven by the need to add the before-mentioned inputHeightsConsideredHot Setting.

In my opinion, we could save a LOT of effort implementing the change proposed by this Issue by simply making one breaking change:

All temperatures in Settings file will be considered "cold".

This is a breaking change, but certainly it's one we expect to make at some point anyway, right?

ntouran commented 2 years ago

Super high level: the state objects of the reactor subpackages should not ever need any information about the initial input at all. The entire state should be reflected as state variables, regardless of how it shows up. This way, models can work made from inputs, made programmatically, made by machine learning, made from databases. I will try to understand the need for this request in #657.

Meanwhile, of the options proposed, I strongly prefer adding another setting to the existing setOptionsFromCs on the core and then using getAncestor to get it. We maybe can move the .core property from Block to ArmiObject to make it more convenient. Then you can access thing.core._mySpecialSetting.

the parent.parent.parent class of options won't work because the hierarchy of an ARMI reactor isn't always consistent. We're adding more options to have blocks with components that have extra children, and component groups (#525) and other related things. You don't want to hard-code a tree structure assumption into an arbitrary tree.

We once had cs available on all ArmiObject subclasses. It ended up leading to massive performance issues and memory leaks. For example when you do a mpi broadcast of the reactor, the system deepcopies everything, and so we ended up with thousands of copies of the cs in RAM and then sent over the network. It was horrifying and painful and ground our supercomputers to a halt. Everyone complained. We spent months understanding and re-optimizing the situation. We vowed never again to put those kinds of attributes on the reactor model. (notably weakrefs are another potentially interesting solution to this class of problem).

I'd also prefer to not make the breaking change at this time. We should shake down this new feature in production more before committing fully to it. We could make that change in the near future, as long as there's no design-specific things that can't be turned completely off by other users.

albeanth commented 2 years ago

@ntouran I appreciate the thorough feedback! Getting context on not subclassing ArmiOject is really interesting. After working with @john-science, I ended up finding a workaround for #657 that didn't require giving ArmiObject children access to the attributes of CaseSetting.

We can use the new inputHeightsConsideredHot setting to more thoroughly test out the cold/hot temperature requirement and revisit the breaking change sometime in the future. Thanks, again!