-
Notifications
You must be signed in to change notification settings - Fork 90
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 better composite iteration methods #2031
base: main
Are you sure you want to change the base?
Conversation
02785b6
to
ab3fdf2
Compare
This feature is definitely big enough for a release note. Since we are planning this PR for just post-release, it should go under Lines 5 to 15 in 68268c1
|
Okay, on first pass I like this. It seems straight forward, and I don't see any problems. I'm going to hold off until Monday to give it a "final review", and hopefully merge it. #fingers-crossed Nice work! |
Produces an iterable over all children that match the flag spec. Composite.getChildrenWithFlags throws these into a list so the user API is the same. Test added showing it does what we expect it to do. Replaced one usage of iteration over the list flavor with the iterable variant.
Rather than generate a potentially large list just to use the first element, try to advance the iterator. If it raises a StopIteration error, we don't have any blocks that match the spec.
I cannot find any uses of this method outside of the one method where it is tested
Calling SFP.getChildren with a flags argument should really be SFP.getChildrenWithFlags
Rather than checking just the length of things, compare we get the objects we expect
Came up in some debugging that this isn't totally tested. Turns out a couple places override this and that was causing my problem. But a test is still good to add!
This should still pass on main. But I added it during some debugging because we didn't have a test just on this method. Tests are still good though
Produces an iterator of the children rather than a list of all children. Does not support the `includeMaterials` option
Special handling for `includeMaterials=True`
Don't need `getChildren` either because `Composite.getChildren` just needs `Composite.__iter__` to be implemented for the traversal.
A lot of places have the pattern ```python for child in self.getChildren() ``` that isn't necessary. If you want to iterate over the children of a composite, you can use `Composite.__iter__` for natural iteration.
We just need a list of assemblies. `list(SFP)` will do that for us
9249016
to
7252468
Compare
@drewj-tp - are there any immediate performance gains from this change that you can share in memory and/or speed? I like the idea of iterators, but I'm curious from a macroscopic perspective of managing the entire reactor how this propagates. Thanks! |
Good questions. I ran some tests on the FFTF model and, when traversing the entire reactor tree:
We'll still see some benefit if users don't migrate to the new Note To see the real changes, I reverted the changes to Memoryimport pathlib
import pickle
import armi
import tracemalloc
armi.configure()
pickleJar = pathlib.Path("fftf.pkl")
if pickleJar.exists():
REACTOR = pickle.loads(pickleJar.read_bytes())
else:
REACTOR = armi.init(fName="fftf/FFTF.yaml").r
pickleJar.write_bytes(pickle.dumps(REACTOR))
tracemalloc.start()
for _ in REACTOR.getChildren(deep=True):
pass
firstSize, firstPeak = tracemalloc.get_traced_memory()
tracemalloc.reset_peak()
for _ in REACTOR.iterChildren(deep=True):
pass
secondSize, secondPeak = tracemalloc.get_traced_memory()
print(f"{firstSize=} :: {firstPeak=}")
print(f"{secondSize=} :: {secondPeak=}")
print(f"{firstSize/secondSize=} :: {firstPeak/secondPeak=}") Timeimport pathlib
import pickle
import armi
import timeit
armi.configure()
pickleJar = pathlib.Path("fftf.pkl")
if pickleJar.exists():
REACTOR = pickle.loads(pickleJar.read_bytes())
else:
REACTOR = armi.init(fName="fftf/FFTF.yaml").r
pickleJar.write_bytes(pickle.dumps(REACTOR))
def getChildren(r):
list(r.getChildren(deep=True))
def iterChildren(r):
list(r.iterChildren(deep=True))
N = 500
FAKE_GLOBALS = locals()
tGet = timeit.timeit("getChildren(REACTOR)", number=N, globals=FAKE_GLOBALS) / N
tIter = timeit.timeit("iterChildren(REACTOR)", number=N, globals=FAKE_GLOBALS) / N
print(f"{tGet=}")
print(f"{tIter=}")
print(f"{tIter/tGet=}") |
That's awesome! I would always argue strongly that you will get more performance improvements if/when we elbow the downstream repositories to start using these new tools. Hell, we could grep around and open the PRs ourselves. Just sayin. |
I have a follow on PR here that will help and then I'll start pinging projects on improvements |
@drewj-tp Love it! I have a few small questions for you, at your leisure. But I love the feature. Assuming your downstream testing suite passes, I'm convinced. |
Once we have `ArmiObject.iterChildren` specified on `ArmiObject`, we can use that to build more base functionality: - `ArmiObject.iterChildrenWithFlags` - `ArmiObject.getChildrenWithFlags` - `ArmiObject.iterChildrenOfType` - `ArmiObject.getChildrenOfType` Additionally, I noted that `ArmiObject.getChildrenWithFlags` had a different default `exactMatch=True` than `Composite.getChildrenWithFlags`. Both now default to `exactMatch=False` for consistency.
Internal tests are passing! |
@@ -1191,7 +1204,11 @@ def countBlocksWithFlags(self, blockTypeSpec=None): | |||
blockCounter : int | |||
number of blocks of this type | |||
""" | |||
return len(self.getBlocks(blockTypeSpec)) | |||
if blockTypeSpec is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your fault, but can we fix the docstring in this method? It says:
Returns the block at a specified axial dimension elevation (given in cm).
But it looks like it should say:
Returns the number of blocks at a specified axial dimension elevation (given in cm).
Also, we can just delete this text from the docstring:
Used as a way to determine what block the control rod will be modifying with a mergeBlocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it!
Great change.
We'll merge when we've looked at downstream testing.
What is the change?
The high-level idea is to provide ways for users and developers to traverse the composite tree without using lists. For example, calling
first traverses the entire tree and populates a list of all composites in the reactor tree. Then, it returns that list to the caller for iteration.
This PR adds the following
Composite
methods:iterChildren
iterChildrenWithFlags
iterChildrenOfType
iterChildrenWithMaterials
Composite
method, but I can't easily think of a better place for it to liveThis PR adds the following specific methods
Assembly.iterBlocks
The various
get*
methods still return lists of things for backwards compatibility, but rely on theiter*
methods. IdeallyComposite.getChildren
would just belist(self.getChildren(...))
but we need to supportComposite.getChildren(..., includeMaterials=True)
so that's some additional complexity.In a few places where we use the
get*
methods for once-through iteration, those are replaced withiter*
. So our example above would instead look likThere are still some use cases for making a list, especially if you want to iterate through the thing twice. Something like
produces an empty list for
second
because the iteratorobjs
is exhausted in the loop.Why is the change being made?
Efficiency, both time (remove redundant traversal over tree) and space (remove list of children)
Closes #1915
Checklist
doc
folder.pyproject.toml
.