Add 1D Cylindrical Modeling to the XS Group Manager #1238
Conversation
control rod modeling to ensure that number densities do not migrate to other components. This is avoided by building a representative block and setting number densities directly onto the components instead of using `b.setNumberDensities()`. Setting number densities directly on the blocks is not a good technique if the blocks are to be treated heterogeneously because there is a chance that material will migrate to other regions based on the "dehomogenization" logic that is applied. Some TODO clean up on using Enums.
# Conflicts: # armi/physics/neutronics/latticePhysics/latticePhysicsInterface.py # armi/physics/neutronics/latticePhysics/tests/test_latticeInterface.py
armi/physics/neutronics/latticePhysics/latticePhysicsInterface.py
Outdated
Show resolved
Hide resolved
|
Once Mike's comments are addressed and the coverage is up I will provide a secondary review. Thanks, Jake! |
armi/physics/neutronics/latticePhysics/latticePhysicsInterface.py
Outdated
Show resolved
Hide resolved
|
@albeanth I added unit tests. There's some coverage spray but coverage still increased overall in spite of that. |
|
This PR is ready, but I am converting to draft while we figure out downstream merge dependencies. |
| info.append((b.getAverageTempInC(), b.getName(), b)) | ||
| info.sort() | ||
| medianBlockData = info[len(info) // 2] | ||
| return medianBlockData[-1] |
There was a problem hiding this comment.
Why the median block temperature?
There was a problem hiding this comment.
I'm not sure, this is what @jakehader originally implemented. We do have a MedianBlockCollection but it's based on the median burnup, not the median temperature. Average probably does make more sense for temperature.
Edit: Although I guess it doesn't matter because then we just deepcopy it and then set the number densities based on the whole block collection. So it's probably not too important which block gets selected here.
There was a problem hiding this comment.
So it's probably not too important which block gets selected here.
Agreed. You could probably simplify _selectCandidateBlock to use just the first block in getCandidateBlocks(). For what it's worth, the unit test within this PR still works with that change.
| self._TWO_DIMENSIONAL_GEOM = "2D" | ||
| self._SLAB_MODEL = " slab" | ||
| self._CYLINDER_MODEL = " cylinder" | ||
| self._HEX_MODEL = " hex" |
There was a problem hiding this comment.
Are these removed because they are unused?
There was a problem hiding this comment.
I see they were used downstream but no longer are. Ok.
There was a problem hiding this comment.
They have been replaced by the XSGeometryTypes Enum. This could affect other users' downstream repos that use the latticePhysicsInterface, but it seems like good practice to force the usage of XSGeometryTypes throughout.
There was a problem hiding this comment.
@sombrereau @drewj-usnctech tagging for visibility and a heads up
|
@mgjarrett @jakehader sorry for the delayed review. This is a particular section of the framework that I have yet to spend a lot of time, so it was newer territory for me and I went a little slower. Also, I think it would have helped expedite the review if either 1) more context was given in the PR description (e.g., from a high-level what changes were being made to each file or groups of files and why), or 2) rebase the commits with sufficient comments in the commits so that they are in nice chunks and can be reviewed sequentially. In its current state it just took some time to parse through and wrap my head around. |
Description
Implement a block collection type for managing 1D cylindrical
control rod modeling to ensure that number densities do not migrate
to other components. This is avoided by building a representative
block and setting number densities directly onto the components
instead of using
b.setNumberDensities(). Setting number densitiesdirectly on the blocks is not a good technique if the blocks are to
be treated heterogeneously because there is a chance that material
will migrate to other regions based on the "dehomogenization" logic
that is applied.
Checklist
doc/release/0.X.rst) are up-to-date with any bug fixes or new features.docfolder.setup.py.