armi icon indicating copy to clipboard operation
armi copied to clipboard

Allow axial expansion related classes to be more extensible

Open drewj-tp opened this issue 1 year ago • 4 comments

I'm working on some internal axial expansion work and finding myself needing to modify some of the functionality on the base class. In some cases I can call super and be good, but in other places I need a bit more control. I've got some partially developed code that I wanted to present in a ticket before fully submitting the PR. Estimating 10/6 for draft PR.

ExpanionData.computeThermalExpansionFactors

One example is modifications to how ExpansionData computes expansion factors for a given component. The current logic for this method is fully encased in a single method

https://github.com/terrapower/armi/blob/34619dfa7991454d45d5d61e9002bea766a353a3/armi/reactor/converters/axialExpansionChanger/expansionData.py#L174-L190

but there isn't a simple way to drop in more bespoke expansion into this method without rewriting the entire routine. So, I propose cracking this method specifically into one primary method and two helper methods, more or less like

def  computeThermalExpansionFactors(self):
    for b in self._a:
        self._blockThermalExpansionFactors(b)  # name TBD
def _blockThermalExpansionFactors(self, b):
    for c in getSolidComponents(b):
        self._componentThermalExpansionFactors(c)
def _componentThermalExpansionFactors(self, c):
    # more or less the internals of the current implementation

Assembly axial link check

The logic for determining if two components are axially linked is vital for determining how the tops and bottoms of blocks move through expansion. This logic is performed in _determinedLinked and isn't really overridable from within

https://github.com/terrapower/armi/blob/34619dfa7991454d45d5d61e9002bea766a353a3/armi/reactor/converters/axialExpansionChanger/assemblyAxialLinkage.py#L186-L200

I propose a staticmethod that points back to that function. Subclasses can use or override this method if they need additional logic for determining component linking.

Small data class for axial links

The data structure for the linking between blocks and between components is dict[T, list[T | None, T | None]] so there's lines like

https://github.com/terrapower/armi/blob/34619dfa7991454d45d5d61e9002bea766a353a3/armi/reactor/converters/axialExpansionChanger/axialExpansionChanger.py#L325

where it may not be immediately clear (outside of some neighboring comments) what position [0] in this returned value represents.

I would like to add a small dataclass like

class Link:
    lower: T | None
    upper: T | None

(type T standin) that would make that above code snippet (and others) look like

b.p.zbottom = self.linked.linkedBlocks[b].lower.p.ztop

that's more readable in my opinion.

Other considerations

  1. Provide an iterator form of getSolidComponents since we mostly use it for iteration - #1915
  2. Combine check for zero-valued and negative expansion factors - https://github.com/terrapower/armi/pull/1861#discussion_r1751058614
  3. https://github.com/terrapower/armi/issues/1453

drewj-tp avatar Sep 30 '24 21:09 drewj-tp

Whatever you end up implementing here (I did not read this ticket in extreme detail), please make sure you consider this PR: https://github.com/terrapower/armi/pull/1376

That PR was closed because for whatever reason we haven't pulled the trigger on it, and it became stale. But we very much intend to merge it or something similar to it into the framework in the near future.

Maybe file that under your "Other considerations" section above.

keckler avatar Sep 30 '24 21:09 keckler

That PR was closed because for whatever reason we haven't pulled the trigger on it, and it became stale. But we very much intend to merge it or something similar to it into the framework in the near future.

@keckler don't you worry! It'll come back ~soon~! It's on the (edge of the) radar

albeanth avatar Sep 30 '24 22:09 albeanth

Whatever you end up implementing here (I did not read this ticket in extreme detail), please make sure you consider this PR: https://github.com/terrapower/armi/pull/1376

I think this will introduce functionality to help improve / extend the component linking. I'll make a comment on the changes I think will be useful and we can chat about some solutions or workarounds

drewj-tp avatar Sep 30 '24 23:09 drewj-tp

@drewj-tp what you propose for computing the thermal expansion factors sounds good to me.

for assembly axial link check -- if you are proposing splitting up _getLinkedComponents into smaller methods for extensibility improvements, cool. yeah that would be nice.

for the data class -- sure. Yeah, that would be nice. You are effectively replacing the 0/1 indexing for lower/upper; which is undoubtedly more readable.

albeanth avatar Oct 04 '24 18:10 albeanth