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.

Sign up for GitHub

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

Jump to bottom

Provide more Composite.iter* methods #1915

Open
drewj-tp opened this issue Sep 30, 2024 · 4 comments · May be fixed by #2031
Open

Provide more Composite.iter* methods #1915

drewj-tp opened this issue Sep 30, 2024 · 4 comments · May be fixed by #2031
Assignees
@drewj-tp
Labels
enhancement New feature or request feature request Smaller user request

Comments

@drewj-tp
Copy link
Contributor

ARMI does a lot of iteration over the composite tree. Understandably so, we have lots of methods that support iterating over children of an object, with some filtering. These usually follow the pattern of parent.getChildren* like Composite.getChildren, Composite.getChildrenWithFlags, and Composite.getChildrenOfType, further expanded to things like Assembly.getBlocks.

A lot of instances of the usage of these get* methods are for iteration

armi/armi/reactor/components/component.py

Line 965 in 96a5c88

for child in self.parent.getChildren():

armi/armi/reactor/converters/axialExpansionChanger/expansionData.py

Line 253 in 96a5c88

componentWFlag = [c for c in b.getChildren() if c.hasFlags(targetFlag)]

armi/armi/reactor/blocks.py

Line 633 in 96a5c88

for c in self.getChildren():

The tradeoff is these get* methods create and populate lists, and then return those lists back to the user. This is great if you need to do multiple iterations or check sizing of the returned object, but it's inefficient for iteration.

I propose we provide iter* methods with similar signatures as their get* counterparts that do not return lists of things. Instead, they could yield back components, produce a filter object, etc. something that allows one-pass iteration over things. The get* methods could then be distilled to

def getChildrenWithFlags(self, ...):
    return list(self.iterChildrenWithFlags(...))

for backwards compatibility.

Candidate methods

To keep the scope clear, I propose we add, test, and use (where appropriate)

  • Composite.iterChildren
  • Composite.iterChildrenWithFlags
  • Composite.iterChildrenOfType

There are other composite subclasses that I believe could benefit from more iteration methods. But for start, I'll keep the scope limited to Composite methods
@drewj-tp drewj-tp added enhancement New feature or request feature request Smaller user request labels Sep 30, 2024
@drewj-tp drewj-tp self-assigned this Sep 30, 2024
@drewj-tp
Copy link
Contributor Author

Future work

Things that could be looked at later

  • Assembly.iterBlocks
  • Assembly.iterBlocksBetweenHeights
  • Block.iterComponentsThatAreLinkedTo
  • Core.iterAssemblies
  • Core.iterAssembliesInRing
  • Core.iterBlocks

Non-exhaustive

@drewj-tp drewj-tp mentioned this issue Sep 30, 2024
Closed
@john-science
Copy link
Member 

Things that could be looked at later

    Assembly.iterBlocks
    Assembly.iterBlocksBetweenHeights
    Block.iterComponentsThatAreLinkedTo
    Core.iterAssemblies
    Core.iterAssembliesInRing
    Core.iterBlocks

If I were to be choosy, I would do Core.iterAssemblies and Core.iterAssembliesInRing last. In an ideal world I would wait until we separate out Core and HexCore or something.

Just thinking aloud.

@drewj-tp
Copy link
Contributor Author

drewj-tp commented Oct 1, 2024

If I were to be choosy, I would do Core.iterAssemblies and Core.iterAssembliesInRing last. In an ideal world I would wait until we separate out Core and HexCore or something.

Agree. I don't intend on tackling things outside Composite.iter* in the first go

@drewj-tp
Copy link
Contributor Author

drewj-tp commented Oct 2, 2024

Some additional digging.

Composite.getChildren has an option that also adds the materials to the list of returned things found in the recursive dig. That feels weird.

That mode, includeMaterials=True is only used twice in armi and internal projects I can access. Both instances are for MPI related distribution and syncing

armi/armi/reactor/composites.py

Line 2778 in f1f3dbe

allComps = [self] + self.getChildren(deep=True, includeMaterials=True)

armi/armi/reactor/composites.py

Line 2884 in f1f3dbe

for child in [self] + self.getChildren(deep=True, includeMaterials=True):

But Composite.getChildren is used heavily! And a lot of iteration, e.g., for c in thing.getChildren. But in a lot of cases, where no arguments are provided, it seems to be a way to just iterate over the children of a composite. Which is naturally supported with __iter__

armi/armi/reactor/composites.py

Lines 2535 to 2536 in f1f3dbe

def __iter__(self):
return iter(self._children)

There are few recursive usage of getChildren, like for VTK visualization

armi/armi/bookkeeping/visualization/vtk.py

Lines 100 to 103 in f1f3dbe

blks = r.getChildren(deep=True, predicate=lambda o: isinstance(o, blocks.Block))
assems = r.getChildren(
deep=True, predicate=lambda o: isinstance(o, assemblies.Assembly)
)

So maybe some wide-spread improvements could be

  1. Replace single-level, material-less usage of for child in parent.getChildren with plain iteration for child in parent.
  2. Recursive iterator that digs through the composite tree, completely or at a specific generationNum
  3. Update the recursive getChildren, with materials, to use the recursive iterator

@drewj-tp drewj-tp linked a pull request Dec 11, 2024 that will close this issue
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@drewj-tp drewj-tp

Labels
enhancement New feature or request feature request Smaller user request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Provide better composite iteration methods terrapower/armi
2 participants
@john-science @drewj-tp