Write and read material theoretical density to/from database #1852
Conversation
|
I have not done a detailed review of this yet, but a few comments:
Anyways, thank you for the PR and for the thought that you put into it. Clearly this is not the simplest problem to solve! |
|
Thanks @keckler. Lots of good points here. Definitely feel the connection to #1062, in addition to #1140 and #1819: materials are not as stateless as we want them to be. And I think that's the deeper problem you're referencing. And that's a larger effort than this symptom (td_frac) but I'm open to discussing with you, @john-science @opotowsky and @albeanth.
Yesish? #1440 touches on TD_frac as a symptom of the statefulness. This PR doesn't close that entire issue but does address a part of it.
This should be transparent to materials. It just reinforces / builds the connection between BP -> material modifications -> TD frac attribute <-> component property <-> database. I'll double check |
|
Chatted w/ @john-science and a maybe better solution is to have This could also coincide w/ some deprecation warnings about removing the material td frac related things. But we'd need to make sure we shift the scaling by TD from the material side to the component side. e.g., from # usage by components
dens = self.material.density()
# materials.py
def density(...):
return actualDensity * self.getTD()to # usage
dens = self.getTD() * self.material.density()
# materials.py
def density(...):
return actualDensityWhich wouldn't be the worst change. Just would need to make sure the change got propogated well. |
|
@keckler @john-science I updated the approach to have Should be good to go! |
|
Had a chat with @keckler this morning and we both agree the recent changes here are better but still have some yuck. His main critique (correct me if I'm wrong) is that we have multiple sources of truth for the density. And Therefore, if you were to examine a "from blueprints" reactor with a modified theoretical density compared to a "from database" reactor at the same timestep,
A solution we are circling around involves discouraging calls to >>> b = r.core[0][0]
>>> c = b[0]
>>> dens = c.material.density(...)
[warn] The density of a component is correct only from that component, not its materialWhere the warning is logged if the density of a material with a parent is called. We'd probably want to do something to consume this warning when we are initializing the component and/or for the first time. Somewhere around armi/armi/reactor/components/component.py Lines 1263 to 1271 in 9ea428b But we also have a non-zero number of tests that look at the agreement between And, what do we do about methods like armi/armi/materials/material.py Lines 653 to 674 in 9ea428b that are convenience methods but still subject to this problem We'd also need to make sure internal projects (and external users!) are avoiding calls to |
|
Related to #1436 |
drewj-tp
force-pushed
the
drewj/serialize-td-frac/1440
branch
from
a1740e9
to
71bacf1
Compare
|
PR builds are passing but push PRs are failing?
both failures are due to xs library read/write? FAILED armi/nuclearDataIO/tests/test_xsLibraries.py::Isotxs_merge_Tests::test_cannotMergeXSLibWithSameNuclideNames - OSError: Failed to read/write <IsotxsIO /Users/runner/work/armi/armi/armi/nuclearDataIO/tests/fixtures/ISOAB>
Traceback (most recent call last):
File "/Users/runner/work/armi/armi/armi/nuclearDataIO/cccc/isotxs.py", line 329, in readWrite
nuclideIO.rwNuclide()
File "/Users/runner/work/armi/armi/armi/nuclearDataIO/cccc/isotxs.py", line 483, in rwNuclide
self._rw7DRecord(blockNumIndex, subBlock)
File "/Users/runner/work/armi/armi/armi/nuclearDataIO/cccc/isotxs.py", line 686, in _rw7DRecord
dataVals.append(record.rwFloat(0.0))
^^^^^^^^^^^^^^^^^^^
File "/Users/runner/work/armi/armi/armi/nuclearDataIO/cccc/cccc.py", line 381, in rwFloat
(f,) = struct.unpack("f", self._stream.read(self._floatSize))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
struct.error: unpack requires a buffer of 4 bytes🤷 |
Well, we can't have tests that only pass some of the time. That's no good. Maybe we'll run them 10 more times and see if they pass. |
|
A Linux unit test just failed: FAILED armi/nuclearDataIO/tests/test_xsLibraries.py::Isotxs_merge_Tests::test_canRemoveIsotopes - OSError: Failed to read/write <IsotxsIO /home/runner/work/armi/armi/armi/nuclearDataIO/tests/fixtures/ISOAB>
Traceback (most recent call last):
File "/home/runner/work/armi/armi/armi/nuclearDataIO/cccc/isotxs.py", line 329, in readWrite
nuclideIO.rwNuclide()
File "/home/runner/work/armi/armi/armi/nuclearDataIO/cccc/isotxs.py", line 483, in rwNuclide
self._rw7DRecord(blockNumIndex, subBlock)
File "/home/runner/work/armi/armi/armi/nuclearDataIO/cccc/isotxs.py", line 686, in _rw7DRecord
dataVals.append(record.rwFloat(0.0))
File "/home/runner/work/armi/armi/armi/nuclearDataIO/cccc/cccc.py", line 381, in rwFloat
(f,) = struct.unpack("f", self._stream.read(self._floatSize))
struct.error: unpack requires a buffer of 4 bytes
===== 1 failed, 2273 passed, 19 skipped, 26 warnings in 264.15s (0:04:24) ======Interesting. |
Very interesting. Same errors I saw in #1852 (comment) Maybe it's a race condition w/ the parallel tests? https://github.com/terrapower/armi/actions/runs/10797243037/job/29948075116?pr=1852#step:6:409 OSError: Failed to read/write <IsotxsIO /home/runner/work/armi/armi/armi/nuclearDataIO/tests/fixtures/ISOAB> |
Defaults to one and will need to be updated in at least two modes 1. Creating from blueprints needs to pull from material, 2. Loading from database needs to assign to material. Unfortunatly, the `setter` function does not provide access to the `Component` so we can't do `self.component.material.assignTD` when assigning a new value to the parameter.
A zero theoretical density might be something to avoid though. It's non-physical except for a void material where there is literally nothing...
I spent a while trying to figure out why the armi object attached to a parameter collection was not the same as the object that held the parameter collection. Or, why `self.p._ArmiObject is not self` evaluates to False but `isinstance(self, self.p._ArmiObject)` does evalute to True. The comment attached to this attribute mentions it is a class but so much of armi revolves around this `node.parent` tree-like behavior that I felt or wanted to have this tree-behavior extend to parameter collections. This would make some work on the TD fraction easier because the parameter setter would have easier access to the `ArmiObject` **instance** that held that collection.
drewj-tp
force-pushed
the
drewj/serialize-td-frac/1440
branch
from
ee618ac
to
88fa540
Compare
|
In the failed instances of this test, the following have all failed
What can we learn from this?
It's possible that separate threads are trying to read from the same fixture resource at the same time. But that should be okay. I've launched a bunch of separate threads all reading from the same file on linux and there are no errors with multiple processes reading the same file. I updated the script to have some processes open the file for writing and some for reading. That also seems to be working, at least in terms of "the operating system does not yell at me." So if multiple parallel workers are trying to read from these fixtures at the same time, we should be good. If, and this is an unfounded if, some worker is writing to the fixture file while another process is reading it, and the fixture file is not fully complete by the time the reader process tries to read, then we could see these errors about not getting all the right bytes for a float that we see. But these fixtures are tracked in the repository and should not be edited during tests. The tests in question create separate empty library objects and do merge actions on those empty libraries with fixture libraries. The error traceback sure looks like we're trying to write a value no matter what in armi/armi/nuclearDataIO/cccc/isotxs.py Lines 685 to 686 in 0b3c7c1 despite the docstring saying it's only trying to read the record armi/armi/nuclearDataIO/cccc/isotxs.py Lines 603 to 605 in 0b3c7c1 The files are also opened for reading in binary only during the well named Test class set upThis might be a red herring. But the test setup for the library merging tests are strange? armi/armi/nuclearDataIO/tests/test_xsLibraries.py Lines 299 to 328 in 0b3c7c1 There's a But then the |
(cherry picked from commit 555cbd1)
|
Okay, we believe the strange test errors were not related to this branch. And we have fixed them in |
Co-authored-by: John Stilley <[email protected]>
|
Test cases found the warning message produced a quarter of a million log entries. Draft while I 1) reduce that, and 2) find a more helpful way to track down calls to |
Using traceback, we can find the thing that called `Material.density` when the material is attached to a `Component`. This information, specifically the file name and line number, is used in the warning message. The file name and line number is used as the label in the warning so multiple locations that invoke this behavior will show up as one logged message. But their number of invocations will also be counted.
|
@john-science @keckler 8d6f624 introduced a mechanism to both reduce the number of log entries and provide a way to track down offending calls |
| return design.assemblies["fuel a"] | ||
| return design.assemblies[assem] | ||
|
|
||
| def loadB4CAssembly(self, materialModifications: str): |
There was a problem hiding this comment.
I'm not convinced this method should exist. It's a one-line method, that just calls another method, and it is only used once.
|
|
||
| def loadUZrAssembly(self, materialModifications): | ||
| yamlString = self.uZrInput + "\n" + materialModifications | ||
| return self._loadAssembly(self.uZrInput, materialModifications, "fuel a") |
There was a problem hiding this comment.
I'm not convinced this method should exist. It's a one-line method, that just calls another method, and it is only used once.
What is the change?
Added a new component parameter
theoreticalDensityFrac. When loading from blueprints, the material's theoretical density is assigned to this parameter. Since it's a parameter, it will be written to the database and then loaded onto the component when reading from db. But, we need to reassign the theoretical density to the material because a lot ofMaterial.densitycalls use this value.This PR adds logic to the database loading sequence to apply the theoretical density component parameter to the material's theoretical density.
This PR also wraps
Material.densityin a decorator that will prefer to callself.parent.densitywhen applicable.Why is the change being made?
Closes #1863
Checklist
docfolder.pyproject.toml.