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

Provide more Composite.iter* methods #1915

Open
drewj-tp opened this issue Sep 30, 2024 · 4 comments
Open

Provide more Composite.iter* methods #1915

drewj-tp opened this issue Sep 30, 2024 · 4 comments
Assignees
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

for child in self.parent.getChildren():

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

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

@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

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

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__

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

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

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

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

No branches or pull requests

2 participants