-
Notifications
You must be signed in to change notification settings - Fork 621
Add properties to settings w/ documentation, c++ loading of filename, and python round-trip test #3808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add properties to settings w/ documentation, c++ loading of filename, and python round-trip test #3808
Changes from 37 commits
8b1b8dc
8713fe9
76c258e
0a7636b
9942d78
886c641
fd1594a
5852ec0
a9b3f28
1b78b84
cc10db6
cf9a27e
4f796fb
6a862b6
cfcbfc2
fb22bba
9a20d6e
015df87
d8c92fd
99ca76f
a86f00e
5474147
902e749
a62b545
c7a8090
ebbdc1a
6355305
19abae7
1726602
0e9eea3
41317cf
cde42e1
17aa7ef
86ec40c
14df979
bcea94f
6ad63cd
f74b535
eb59f5a
ab73212
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -179,6 +179,9 @@ class Settings: | |
| Initial seed for randomly generated plot colors. | ||
| ptables : bool | ||
| Determine whether probability tables are used. | ||
| properties_file : Pathlike | ||
| Location of the properties file to load cell temperatures/densities | ||
| and materials | ||
| random_ray : dict | ||
| Options for configuring the random ray solver. Acceptable keys are: | ||
|
|
||
|
|
@@ -392,6 +395,7 @@ def __init__(self, **kwargs): | |
| self._photon_transport = None | ||
| self._plot_seed = None | ||
| self._ptables = None | ||
| self._properties_file = None | ||
| self._uniform_source_sampling = None | ||
| self._seed = None | ||
| self._stride = None | ||
|
|
@@ -1018,6 +1022,18 @@ def temperature(self, temperature: dict): | |
|
|
||
| self._temperature = temperature | ||
|
|
||
| @property | ||
| def properties_file(self) -> PathLike | None: | ||
| return self._properties_file | ||
|
|
||
| @properties_file.setter | ||
| def properties_file(self, value: PathLike | None): | ||
| if value is None: | ||
| self._properties_file = None | ||
| else: | ||
| cv.check_type('properties file', value, PathLike) | ||
| self._properties_file = input_path(value) | ||
|
|
||
| @property | ||
| def trace(self) -> Iterable: | ||
| return self._trace | ||
|
|
@@ -1708,6 +1724,12 @@ def _create_temperature_subelements(self, root): | |
| else: | ||
| element.text = str(value) | ||
|
|
||
| def _create_properties_file_element(self, root): | ||
| if self.properties_file is not None: | ||
| element = ET.Element("properties") | ||
| element.text = str(self.properties_file) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something went wrong in the latest update @gonuke, I think it's here (i.e there's nothing called which leads to this behavior I think you either want to use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm kind of confused why the test didn't fail though
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually // read properties from file
if (check_for_node(root, "properties")) {
properties_file = get_node_value(root, "properties");
if (!file_exists(properties_file)) {
fatal_error(fmt::format("File '{}' does not exist.", properties_file));
}
}The test looks for a node called properties, which is why it works. Perhaps now we just call it
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is your executable up to date?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nvm EDIT: It's running now... I'll report back when it completes and hopefully it will be statistically equivalent to cardinal / OpenMC depletion. Also it printed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking much better now! Depletion first transport with properties below and Cardinal gave us If you use 1 sigma, depletion is slightly outside the same CI as the other two, but 2 sigma and they all overlap. However, it maybe makes sense that depletion is more different than the other two, as there's more nuclides loaded (small statistical shifts in sampling) and the other two have the exact same model.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a clearer test would be running this entirely outside of Cardinal and compare:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't need to be very long running for this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I agree (the above results took very long to run), so I wasn't pitching it for a test. Was just confirming that the properties generated by the 180 drum case now agrees between Cardinal, standalone transport using @pshriwise do you think we should add a regression test that computes eigenvalues and compares cases with different / the same properties? I think Paul's idea for a test could work well if we use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't mean to dip into formal "testing" but that also makes sense... I guess I was just thinking it would be a clearer demonstration of the capability than ensuring it matches Cardinal. I guess it would not necessarily demonstrate that it was mapping the temperatures to the correct cells, if we only looked for it to be different from a case without.... |
||
| root.append(element) | ||
|
|
||
| def _create_trace_subelement(self, root): | ||
| if self._trace is not None: | ||
| element = ET.SubElement(root, "trace") | ||
|
|
@@ -2205,6 +2227,11 @@ def _temperature_from_xml_element(self, root): | |
| if text is not None: | ||
| self.temperature['multipole'] = text in ('true', '1') | ||
|
|
||
| def _properties_file_from_xml_element(self, root): | ||
| text = get_text(root, 'properties') | ||
| if text is not None: | ||
| self.properties_file = text | ||
|
|
||
| def _trace_from_xml_element(self, root): | ||
| text = get_elem_list(root, "trace", int) | ||
| if text is not None: | ||
|
|
@@ -2440,6 +2467,7 @@ def to_xml_element(self, mesh_memo=None): | |
| self._create_ifp_n_generation_subelement(element) | ||
| self._create_tabular_legendre_subelements(element) | ||
| self._create_temperature_subelements(element) | ||
| self._create_properties_file_element(element) | ||
| self._create_trace_subelement(element) | ||
| self._create_track_subelement(element) | ||
| self._create_ufs_mesh_subelement(element, mesh_memo) | ||
|
|
@@ -2554,6 +2582,7 @@ def from_xml_element(cls, elem, meshes=None): | |
| settings._ifp_n_generation_from_xml_element(elem) | ||
| settings._tabular_legendre_from_xml_element(elem) | ||
| settings._temperature_from_xml_element(elem) | ||
| settings._properties_file_from_xml_element(elem) | ||
| settings._trace_from_xml_element(elem) | ||
| settings._track_from_xml_element(elem) | ||
| settings._ufs_mesh_from_xml_element(elem, meshes) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.