-
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
Merged
paulromano
merged 40 commits into
openmc-dev:develop
from
gonuke:properties_in_settings
Mar 13, 2026
+129
−4
Merged
Add properties to settings w/ documentation, c++ loading of filename, and python round-trip test #3808
Changes from all commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
8b1b8dc
add properties file path to settings
gonuke 8713fe9
add flags to indicate which properties to read from properties file
gonuke 76c258e
Add new XML reading method to check existence of file path
gonuke 0a7636b
add flags for which properties to read from file
gonuke 9942d78
read properties file after other initialization
gonuke 886c641
propagate boolean for property reads into C API and related python calls
gonuke fd1594a
propagate read properties flags to cells
gonuke 5852ec0
propagate read properties flags to material
gonuke a9b3f28
add new booleans to header
gonuke 1b78b84
apply clang formatting
gonuke cc10db6
fix formatting
gonuke cf9a27e
manually fix clang-format inconsistency
gonuke 4f796fb
fix string to char* conversion
gonuke 6a862b6
declare new XML reader
gonuke cfcbfc2
add missing semi-colon and guess at formatting
gonuke fb22bba
add default value to signature
gonuke 9a20d6e
clang-format-15
gonuke 015df87
add file_utils to xml_interface
gonuke d8c92fd
back away from new XML reader for valid file paths
gonuke 99ca76f
Improve documentation
gonuke a86f00e
switch to C++ error handling
gonuke 5474147
cleanup lib call tests
gonuke 902e749
add temporary file for testing
gonuke a62b545
don't convert back to string in test
gonuke c7a8090
Don't generate file, but do test against Path
gonuke ebbdc1a
abbreviate names
gonuke 6355305
update documentation
gonuke 19abae7
cleanup comments and error messages
gonuke 1726602
simplify early exit logic
gonuke 0e9eea3
style update
gonuke 41317cf
read the properties in the right place
gonuke cde42e1
unwind fine-grained import booleans
gonuke 17aa7ef
Adding manual test for properties load via settings
pshriwise 86ec40c
Abandoning the test fixture as it adds complexity
pshriwise 14df979
Altering TemporarySession intracomm handling for cleaner testing
pshriwise bcea94f
Test code cleanup
pshriwise 6ad63cd
Merge pull request #1 from pshriwise/properties_in_settings
gonuke f74b535
Doc fix
paulromano eb59f5a
Merge branch 'develop' into pr/gonuke/3808
paulromano ab73212
Use properties_file consistently, small fixes
paulromano File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
properties_filein the XML now.) I just re-tried and my settings looks like thiswhich leads to this behavior
I think you either want to use
element.setAttributeto setfilepathor have the code just look for thepropertieselementThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of confused why the test didn't fail though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually
The test looks for a node called properties, which is why it works. Perhaps now we just call it
propertiesinstead ofproperties_filenow that we only supply a fileThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is your executable up to date?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm
I forgot to do make install after make and before installing the python apiEDIT: 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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking much better now!
New transport only (latest commit) below
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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:
properties.h5propreties.h5and show that they are very different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to be very long running for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
model.xml, and depletion first step (which is how we noticed that themodel.xmlversion wasn't loading properties correctly).@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
openmc.lib.export_properties()to make a properties file for testing. While the eigenvalues might change over time due to floating point/OpenMC code updates, the data in the properties file will always be the same. Perhaps a UO2 cylinder divided into two axial regions: 300K and 600K? (I could make a PR into Paul's branch, if so).There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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....