Improve logic for computing thermal expansion factors #1342
Conversation
| @@ -559,12 +560,13 @@ def _determineLinked(componentA, componentB): | |||
| class ExpansionData: | |||
| """Object containing data needed for axial expansion.""" | |||
|
|
|||
| def __init__(self, a, setFuel): | |||
| def __init__(self, a, setFuel: bool, coldHeightsToHot: bool): | |||
There was a problem hiding this comment.
doc string please and describe when coldHeightsToHot would be true and the associated setting.
Also call out that it significantly doesn't preserve mass since component is hot radial dims but heights are cold input dims
There was a problem hiding this comment.
add in doc string too. its a big feature difference than what someone might assume. we dont want someone mis-using in a new case because the doc string doesn't call out the major pitfal
| @@ -192,7 +193,7 @@ def setAssembly(self, a, setFuel=True): | |||
| This is useful when target components within a fuel block need to be determined on-the-fly. | |||
| """ | |||
| self.linked = AssemblyAxialLinkage(a) | |||
| self.expansionData = ExpansionData(a, setFuel) | |||
| self.expansionData = ExpansionData(a, setFuel, coldHeightsToHot) | |||
There was a problem hiding this comment.
prefer when calling to pass kwargs directly
self.expansionData = ExpansionData(a, setFuel=setFuel, coldHeightsToHot=coldHeightsToHot)
in this case if kwargs are re-ordered or one is removed you are still safe
| @@ -666,7 +668,7 @@ class TestDetermineTargetComponent(AxialExpansionTestBase, unittest.TestCase): | |||
|
|
|||
| def setUp(self): | |||
| AxialExpansionTestBase.setUp(self) | |||
| self.expData = ExpansionData([], None) | |||
| self.expData = ExpansionData([], True, True) | |||
There was a problem hiding this comment.
define what the variables are
self.expansionData = ExpansionData(a, setFuel=True, coldHeightsToHot=True)
| @@ -156,6 +156,7 @@ def performThermalAxialExpansion( | |||
| tempGrid: list, | |||
| tempField: list, | |||
| setFuel: bool = True, | |||
| coldHeightsToHot: bool = False, | |||
There was a problem hiding this comment.
Should add coldHeightsToHot to the docstring
There was a problem hiding this comment.
@john-science it'd be nice if our linting would catch this. What do you think?
|
Converting to draft while I figure out downstream merge dependencies. |
| determines if thermal expansion factors should be calculated from c.inputTemperatureInC | ||
| to c.temperatureInC or some other reference temperature and c.temepratureInC |
There was a problem hiding this comment.
its more than that though. its not just reference temperature. its performing non mass conserving expansion to accommodate coldHeightsConsidered Hot
Co-authored-by: Chris Keckler <kecklerct@gmail.com>
Co-authored-by: Chris Keckler <kecklerct@gmail.com>
What is the change?
The logic within axialExpansionChanger.py::ExpansionData::computeThermalExpansionFactors was prone to computing the wrong thermal expansion factors for some corner cases. This PR aims to address these use cases by adding an optional parameter that specifically calls out if the temperature change for computing the thermal expansion factors should use c.inputTemperatureInC and c.temperatureInC (i.e., if
coldHeightsToHotis True) or between the "component reference temperature" and c.temperatureInC (i.e., ifcoldHeightsToHotis False).Why is the change being made?
The logic for computing thermal expansion factors was error prone and could be misinterpreted.
Checklist
doc/release/0.X.rst) are up-to-date with any important changes.docfolder.setup.py.