Add armi.reactor.compositeList.CompositeList : base class for Assemblies #1088
Conversation
| spatialGrid: Optional[Grid] | ||
|
|
||
| def __init__(self, name): | ||
| super().__init__(name) |
There was a problem hiding this comment.
Style points thing.
For reasons that pre-date my involvement in this project, we have a style guide in the docs that says to not use super(), in favor of more explicit calls to the super class __init__() function.
Just to be entirely clear, we use super() all over ARMI, and no one has bothered to fix it yet. So, it's not mission-critical. But probably we should at least leave the Assembly one as-is.
There was a problem hiding this comment.
Roger. We will need to change the assembly __init__ to not point to Composite.__init__ instead the parent class, but I think the spirit of the request would still be preserved
There was a problem hiding this comment.
How do we feel about this in light of #1155?
| def _renameChild(self, child: Composite, index: int): | ||
| """Rename a child at a given index in the list""" | ||
|
|
||
| def calculateZCoords(self): |
There was a problem hiding this comment.
It would be cool if all these methods had their own unit tests.
There was a problem hiding this comment.
Excellent request. Since the Assembly is the primary concrete subclass, and it's well tested through armi/reactor/tests/test_assemblies.py, all if not most of these methods should be tested.
The alternative approach could be to make another child subclass specific for testing the composite list methods. It would look similar to those in the assembly testing, but maybe not tied strictly to blocks? Ah but we need the children to have a getHeight method...
Thoughts?
There was a problem hiding this comment.
Ah, you're counting on the Assembly subclasses unit tests. I see.
Well, in that case, your unit test coverage is pretty high for the new file (~91%).
There was a problem hiding this comment.
If you're alright with it, I'll extend the existing assembly tests to bump the coverage up. There's some edge cases in some pretty public methods (getChildrenAndZ -> getBlocksAndZ) that aren't touched
|
Personal todo
|
|
@john-science other than some conflicts in the release notes, is there anything else you're looking to see? |
armi/reactor/compositeList.py
Outdated
| @@ -0,0 +1,260 @@ | |||
| # Copyright 2019 TerraPower, LLC | |||
There was a problem hiding this comment.
Updated copyright year
The intention of the abstract parent class is to shift most of the composite-tree traversal routines onto a generic parent class. It's abstract only because the Assembly does some re-naming of children during `Assembly.add` which subclasses might want to override. Might not be enough to justify all the metaclasses and just make that an empty method. Later, another subclass of `CompositeList` could be used to model stacks of fuel pellets in a ``Block``. But that would require many blueprint side modifications that are to be discussed. Some assembly methods like `getBlockAndZ` pass directly off to the parent method to maintain the same API. Wondering if there is room to make some of these iterator-based methods a generator? But that would also be an API change and should be done after some discussions. Added a test for getBlocksAtElevation. Closes terrapower#1030 Signed-off-by: Andrew Johnson <a.johnson@usnc-tech.com>
drewj-usnctech
force-pushed
the
drewj-usnctech/1030/assembly-1d-parent-class
branch
from
83cbd1f to
b406975
Compare
|
@john-science getting back to this. Is there anything you'd want to see before calling this change acceptable? |
@drewj-usnctech, @john-science bump! |
|
Going to close this for now because it's dropped off our radar. We might re-investigate this after a few months |
Description
The intention of the abstract parent class is to shift most of the composite-tree traversal routines onto a generic parent class. It's abstract only because the Assembly does some re-naming of children during
Assembly.addwhich subclasses might want to override. Might not be enough to justify all the metaclasses and just make that an empty method.Later, another subclass of
CompositeListcould be used to model stacks of fuel pellets in aBlock. But that would require many blueprint side modifications that are to be discussed.Some assembly methods like
getBlockAndZpass directly off to the parent method to maintain the same API. Wondering if there is room to make some of these iterator-based methods a generator? But that would also be an API change and should be done after some discussions.Added a test for getBlocksAtElevation.
Closes #1030
Checklist
doc/release/0.X.rst) are up-to-date with any bug fixes or new features.docfolder.setup.py.