Allow components to have children in support of particle fuel modeling #702
Conversation
|
I don't have a good enough grasp of these changes to know the answer, but does this change account for this comment you had made about the material modifications? And a further question -- Do we want to allow different material modifications at this additional layer as well? Sounds messy... |
There was a problem hiding this comment.
@ntouran thanks for putting this together. The input file changes look good.
Maybe a hyperbolic situation, but if I had a Component that had two blended component groups, how could I separate out the children? Or maybe a better way to put it: how can plugins know that all (or some) of the children on a component belong together in the same particle fuel component group?
I don't have great answers for these but hopefully the questions are helpful.
| children = {c.name: c for c in constructedObject.getChildren()} | ||
| for child in children.values(): | ||
| child.resolveLinkedDims(children) |
There was a problem hiding this comment.
This enables individual layers to have linked dimensions, right? Nice!
There was a problem hiding this comment.
yeah it should. though we should make more unit tests proving it.
| else: | ||
| if self.hasFlags(typeSpec, exact): | ||
| yield self |
There was a problem hiding this comment.
This removes the ability for component.iterComponents to yield itself. I think
if self.hasFlags(typeSpec, exact):
yield self
# If no children, this loop isn't executed
for child in self._children:
for c in child.iterComponents(typeSpec, exact):
yield cmight pick up the arbitrary depth and also return the child component based on the child's flags not the flags of the parent
There was a problem hiding this comment.
very nice, that does seem better. I'll try your suggestion.
There was a problem hiding this comment.
ok so I tried With that suggested iterComponents change, and my test_trisoCase.py unit test fails with max recursion errors.
I tried this kind of thing for a while and just couldn't quite get it working. I'm sure it can be done... it's just hard.
[c.getNumberDensity(nuc) for nuc in nucNames]
File "/home/nick/repos/armi/armi/reactor/composites.py", line 1256, in <listcomp>
[c.getNumberDensity(nuc) for nuc in nucNames]
File "/home/nick/repos/armi/armi/reactor/composites.py", line 1233, in getNumberDensity
return self.getNuclideNumberDensities([nucName])[0]
File "/home/nick/repos/armi/armi/reactor/components/component.py", line 605, in getNuclideNumberDensities
composites.Composite.getNuclideNumberDensities(self, nucNames),
File "/home/nick/repos/armi/armi/reactor/composites.py", line 1255, in getNuclideNumberDensities
[
File "/home/nick/repos/armi/armi/reactor/composites.py", line 1256, in <listcomp>
[c.getNumberDensity(nuc) for nuc in nucNames]
File "/home/nick/repos/armi/armi/reactor/composites.py", line 1256, in <listcomp>
[c.getNumberDensity(nuc) for nuc in nucNames]
File "/home/nick/repos/armi/armi/reactor/composites.py", line 1233, in getNumberDensity
return self.getNuclideNumberDensities([nucName])[0]
File "/home/nick/repos/armi/armi/reactor/components/component.py", line 605, in getNuclideNumberDensities
composites.Composite.getNuclideNumberDensities(self, nucNames),
File "/home/nick/repos/armi/armi/reactor/composites.py", line 1244, in getNuclideNumberDensities
[
File "/home/nick/repos/armi/armi/reactor/composites.py", line 1245, in <listcomp>
c.getVolume() / (c.parent.getSymmetryFactor() if c.parent else 1.0)
File "/home/nick/repos/armi/armi/reactor/components/component.py", line 430, in getVolume
if self.p.volume is None:
File "/home/nick/repos/armi/armi/reactor/parameters/parameterDefinitions.py", line 293, in __get__
return self._getter(obj)
RecursionError: maximum recursion depth exceeded
There was a problem hiding this comment.
Ooooo that's rough.
Looks like the problem could stem from Composite.getNumberDensity relying on Composite.getNuclideNumberDensities?
armi/tests/refTriso-bp.yaml
Outdated
| component groups: | ||
| triso particle: | ||
| kernel: | ||
| mult: 1000 | ||
| buffer: | ||
| mult: 1000 | ||
| ipyc: | ||
| mult: 1000 | ||
| sic: | ||
| mult: 1000 | ||
| opyc: | ||
| mult: 1000 |
There was a problem hiding this comment.
Should the multiplicity here be 1 for each component? In that there is one kernel in a triso particle but maybe thousands of triso particle components in the triso compact component with the blend.
There was a problem hiding this comment.
well based on the names, what you say seems more right. But the way it's being interpreted has the number of compacts in a block set to 20 and the number of particles per compact set to 1000. I guess I should try to put more realistic numbers in here. Anyway I wonder how I could adjust this to make it more intuitive.
There was a problem hiding this comment.
Having mult attached to the compact matrix component feel intuitive. Then you would also be able to get the multiplicity from a spatial grid.
Is the sticking point that we can't define the mult of the triso particle in the compact with a blend fraction until we know the volume of the matrix?
| if self._children: | ||
| childDens = dict( | ||
| zip( | ||
| nucNames, | ||
| composites.Composite.getNuclideNumberDensities(self, nucNames), | ||
| ) | ||
| ) | ||
| # get volume of children by using the composite method which loops over all children | ||
| childVol = composites.Composite.getVolume(self) | ||
| # get self volume by calling the shape-specific volume method of the background shape | ||
| totalVol = self.getVolume() | ||
| childFrac = childVol / totalVol | ||
| selfFrac = 1.0 - childFrac | ||
| return [ | ||
| self.p.numberDensities.get(nucName, 0.0) * selfFrac | ||
| + childDens.get(nucName, 0.0) * childFrac | ||
| for nucName in nucNames | ||
| ] | ||
| else: | ||
| return [self.p.numberDensities.get(nucName, 0.0) for nucName in nucNames] |
There was a problem hiding this comment.
How would a plugin be able to get the number densities of just the matrix component with this change?
On one hand this is consistent with how the design of the composite tree: Composite.getNuclideNumberDensities gives the densities for all children on a node. So this would match well with Block.getNuclideNumberDensities for some nodal homogenization methods. But modeling this Component exactly in a monte carlo code would be problematic w/o the number densities of just the Component and not its children
There was a problem hiding this comment.
Great point. You would have to access the matrix's .p.numberDensities parameter directly in order to differentiate between it and its children with this change as written. Willing to consider more elegant options.
There was a problem hiding this comment.
@ntouran is
matrix's .p.numberDensities
still the recommended approach here? I've been testing this out over the last few days and had a hard time remembering this thread.
Would something like Component.getNuclideNumberDensities(children=False) be acceptable? I tend to be hesitant pulling data from these parameter containers as the feel (maybe incorrectly) more private than a public method
| return list(self.p.numberDensities.keys()) | ||
| nucs = set(self.p.numberDensities.keys()) | ||
| if self._children: | ||
| childNuclides = set(composites.Composite.getNuclides(self)) | ||
| nucs |= childNuclides | ||
| return nucs |
There was a problem hiding this comment.
Same concern on getting nuclides just in the component vs. component and all the children
There was a problem hiding this comment.
yeah that's a good concern. I'd hate to make you basically re-write these methods for accessing the details of the matrix itself. We could add in a flag or something as a method argument? getNuclides(includeChildren=False) or something?
There was a problem hiding this comment.
includeChildren=False feels like a good solution. Then whatever application or plugin is doing the writing could check if a Component has children prior to writing the material definitions
|
To the question by @keckler (and I might be off base w/ the intent or background) but I'd still hope to have the ability to use the But I don't have a lot of background on the component-wise material modifications in #517 (which looks awesome btw) |
|
Bump. Needs |
|
@ntouran I feel decently good about the internals. My biggest request comes from not requiring the user to input number of particles per compact in the blueprints file. There are some occasions where you might know the exact number of particles in a compact, especially as a design matures. But for more preliminary studies, or where you have compacts of multiple sizes but the same volume fraction, requiring the user to know the number of particles at the start can be burdensome |
ntouran
force-pushed
the
component-children
branch
from
001861d
to
681ee14
Compare
|
@drewj-usnctech ok I have made some small changes that now auto-derive the mult completely based on the blendFrac. So you put in dummy mults of 1.0 and the system updates the kernel mults during init to match the expected blend frac. One weird thing here is that the blendfrac is applied to the hot dimensions at init, (but will stay constant after init even if temperatures change) |
|
@ntouran thanks for the update. I've been out for a bit and am finally catching up.
This was my suspicion as well. We've been having some internal discussion about how thermal expansion impacts our particle fuel modeling and guess this might be a problem. I imagine this component expansion is done very early in the blueprint loading and there isn't a way to delay it? Or wholesale disable it? |
You can disable thermal expansion by setting |
🤦 yeah, I forgot about that when writing that comment. Didn't mean to bring a larger discussion (#766) into this feature. But there is still hiccup if you decide to enable thermal expansion and model particle fuel here. As @ntouran mentioned
This means the number of particles packed into the compact would reflect the post-expansion dimension rather than the as built dimension. There would be an opaque change in fuel loading due to this thermal expansion. FWIW, as the main advocate for this feature, I'm okay saying this pre-expansion should not be a blocker for this feature yet. We're still trying to sort out thermal expansion and the various properties and knock-on effects internally, and are probably a ways from fully diving in. Hence my posting of questions like #766 and #694 |
|
@ntouran This probably just needs to be merged with @drewj-usnctech This PR has been open too long, how do you feel about it? I'd like to move this forward. |
|
@john-science This has been up a while and we've started some internal discussions about adjacent modifications to the composite tree. I don't have anything I can provide at this point, but I'll hopefully be reaching out to the dev team in a week or so about how a larger enhancement. This could capture both the particle fuel modeling and provide some additional modeling benefits. We think. It's still in a conceptual phase and we'd like to give it some more time before sending it off |
|
(fully aware that I am somewhat responsible for the delay) |
ntouran
force-pushed
the
component-children
branch
from
76c2356
to
06b0d85
Compare
|
This is on our task list for things to do this week and report back if we find any thing that seriously blocks / breaks our internal plugins. One thing that I'm thinking about, while I'm thinking about composite tree stuff (#503 // #960), does this break the notion that a |
| if self._children: | ||
| for child in self._children: | ||
| if self.hasFlags(typeSpec, exact): | ||
| yield child | ||
| else: | ||
| if self.hasFlags(typeSpec, exact): | ||
| yield self |
There was a problem hiding this comment.
@ntouran we've found that the mass of the matrix material is not accounted for when doing getMass calls. I think due to this section here
We modified the triso mass test blueprint file such that each element only appears in one Component. Then, block.getMass("C12") (in theory) picks up mass from the triso compact component and should be non-zero. Unfortunately this is not the case 😬
# A full blueprint file that contains assems made with ComponentGroups
blocks:
fuel2: &block_fuel2
triso compact:
shape: Circle
blends:
- triso particle
blendFracs:
- 0.70
material: Graphite
Tinput: 600.0
Thot: 600.0
mult: 1
id: 0.0
od: 1.2446
duct:
shape: Hexagon
material: Cu
Tinput: 600.0
Thot: 600.0
ip: 9.0
mult: 1.0
op: 10.0
coolant:
shape: DerivedShape
material: NaCl
Tinput: 25.0
Thot: 600.0
components:
# triso dims from Table 4 in TRISO benchmark
kernel:
shape: Sphere
material: Lead
Tinput: 600.0
Thot: 600.0
id: 0.0
mult: 1.0
od: 0.010625
buffer:
shape: Sphere
material: Lead
Tinput: 600.0
Thot: 600.0
id: kernel.od
mult: 1.0
od: 0.015625
ipyc:
shape: Sphere
material: Lead
Tinput: 600.0
Thot: 600.0
id: buffer.od
mult: 1.0
od: 0.017375
sic:
shape: Sphere
material: Lead
Tinput: 600.0
Thot: 600.0
id: ipyc.od
mult: 1.0
od: 0.019125
opyc:
shape: Sphere
material: Lead
Tinput: 600.0
Thot: 600.0
id: sic.od
mult: 1.0
od: 0.021125
component groups:
triso particle:
kernel:
mult: 1
buffer:
mult: 1
ipyc:
mult: 1
sic:
mult: 1
opyc:
mult: 1
assemblies:
fuel c: &assembly_c
specifier: OC
blocks: [*block_fuel2]
height: [1.0]
axial mesh points: [1]
xs types: [A]
systems:
core:
grid name: core
origin:
x: 0.0
y: 0.0
z: 0.0
grids:
core:
geom: hex
symmetry: full
lattice map: |
OC
nuclide flags:
B: &nuc_flags {burn: false, xs: true}
C: *nuc_flags
PB: *nuc_flags
CU63: *nuc_flags
CU65: *nuc_flags
CL35: *nuc_flags
CL37: *nuc_flags
CL37: *nuc_flags
NA23: *nuc_flagsThere was a problem hiding this comment.
Thanks @drewj-usnctech I will bring in your modified blueprints file and update the unit test so it fails as you've found and then try to fix.
This updates the iteration and number density methods to expect children and blend them with the component itself as a background composition. This is being developed specifically to help support TRISO particle-type fuel that has important fine structure within the fuel pins.
Initial TRISO unit tests now pass.
This allows users to not need to know anything about mults for e.g. particle fuel and should get the appropriate mults derived during init.
ntouran
force-pushed
the
component-children
branch
from
06b0d85
to
c91bbab
Compare
|
This might be related to the mass check on the matrix, but we also found that the volume of the matrix component did not change when we changed the This feels like a weird composite tree dilemma. If I have a |
|
Yeah so I think I need to re-implement this to follow a more robust version of the Composite design pattern. This will take a bit more work, sorry, but should be more robust. I have been adding to the unit tests and splitting them out to cover a few expected behaviors. We probably will want to add even more. They currently fail, as they should. Current plan to get them running is to:
I think the net result will be better support of arbitrary nesting. |
|
@ntouran sounds like a good plan. Let me know how we can help, either in developing or providing tests / requirements. One question
Are you thinking about having |
ARMI handles non-integer mults. See #542 |
Description
This changes the ARMI data model to allow components to have children. It also adds a input
capability to allow users to input particle type fuel (e.g. TRISO).
This is a proof-of-concept at the moment, intended for getting feedback from @drewj-usnctech
wrt #477 .
Mostly, take a look at the input file and let us know if it satisfies the need. We will need
to add more testing and documentation to verify that everything is working
and explained.
Checklist
If user exposed functionality was added/changed:
docfolder.setup.py.