Export of depletion results to openmc.Materials #1726

gridley · 2020-11-18T22:35:37Z

Builds off #1714

This PR just ties in a few pieces of feedback from the PR mentioned there. Now, OPENMC_CROSS_SECTIONS or the cross_sections element of a materials.xml file is used to determine which nuclides should be added to depleted materials. I could have made a PR to @rockfool's fork, but didn't so you don't have to worry about this Jiankai. And then we could get feedback from Drew and Paul faster on this.

Anyways, this also includes the recommendation of returning a Materials instance, and does not export to XML by default. Rather, if a certain kwarg is provided for a new XML file name, the new materials XML file will be written. The new Materials instance is always returned, though.

Non-burnable materials are also included in the result.

Was there anything else you guys (@paulromano and @drewejohnson) think would be useful for this?

drewejohnson · 2020-11-25T16:04:56Z

Apologies in the delay getting to this. Been a wild few weeks on my end.

To the matter at hand. I'm confused as to what PR should be reviewed for this feature: this or #1714. Is @rockfool is still working on their implementation? I'm not sure how I feel about having two PRs up for the same feature, especially with one being a fork of the other.

I do like the fact that this pulls in information from a materials.xml file, but I'm not sure if that should be the responsibility of this method. I'm still partial to the idea of just extracting the burnable materials, and then updating outside this method - #1714 (comment) A workflow like

r = openmc.deplete.ResultsList.from_hdf(rfile)
burnable = r.export_materials()
all_mats = openmc.Materials.from_xml(mfile)
# or from Materials built by a script and not exported yet
all_mats.update(burnable)
all_mats.export_to_xml("materials.xml")

is more verbose than the current PRs, but I think it yields a lot more control to the user / developer. For the case where new materials are added through burnup (refueling, reprocessing operations? maybe not) these may not exist in a file that has been currently exported. But this also could be some premature optimization / YAGNI

rockfool · 2020-11-25T16:11:15Z

I am also sorry for this delayed follow-up. I think we can proceed with this in parallel. And you (@drewejohnson ) can approve the one you like.

gridley · 2020-11-25T18:38:28Z

Yeah, even in the refueling/reprocessing case, if it's anything like serpent, new materials are never added over the duration of the calculation. So, yeah, I mainly just put in this separate PR since @rockfool probably has lots of other stuff to do aside from address comments on that PR. Guess it should have gone in as a PR to your PR Jiankai, but since there are so few changes, I figured it would be easiest to just put in another.

Anyways, yeah, IMO it's a bit YAGNI to add in that level of generality. If someone comes up on the user list asking for this feature, we'll know what to do--and it won't be very much work to rewrite this to accommodate.

drewejohnson

@rockfool please see the comments here if you wish to incorporate them into #1714

I've made some recommendations for how I believe this PR could be improved. One big thing that both PRs are missing is testing.

I will bring up the modular approach from before one more time, since it would be my preference. That would allow easier testing in my opinion: can we get the burnable materials from the result file. There is some overlap, and the functionality would not be fully complete w/o the Materials.update feature. Which I'm happy to take on

openmc/deplete/results_list.py

gridley · 2020-12-14T19:08:53Z

Could I get some suggestions on how to test this? Not sure if it should be in regression or units. Given the dependence on an initial materials.xml and a depletion statepoint file, maybe this would fit nicely into an existing test. Not familiar enough to say where though.

drewejohnson · 2020-12-16T15:41:16Z

I think our depletion regression test would be the most complete place to test this method. We don't have a materials.xml file there but it's the only place we have a full depletion file. You can probably use some of the test fixtures to grab the model or operator to print the material file.

To explain some of my comments, or the motivation therein, I recently was burned by a depletion simulation where a non-performance critical part ended up taking an inordinate amount of the simulation time. So that was fresh in my mind when writing the review. I'll address the comments directly, but I wanted to put that forward first

gridley · 2020-12-16T16:14:10Z

Ah, that's very interesting Drew, I wouldn't have guessed. Which piece was slowing you down a lot? This piece may be good for optimization if anyone starts doing multiple statepoint calculations at different depletion states. I'll have to test performance on a larger problem at some point.

Anyways, I'll come back to this PR this weekend! Thanks for the prompt responses.

drewejohnson · 2020-12-16T16:29:51Z

To clarify, it wasn't OpenMC. I would be very interested in timing breakdowns of how OpenMC performs when depleting large problems but haven't had the chance to tackle that yet. If you find something interesting, please pass it along

And just ping me when this is ready for another review 🙂

gridley · 2020-12-21T17:55:10Z

@drewejohnson alright, the more specific exceptions are in now, as well as a test for the ResultsList->openmc.Materials conversion. The test just makes a materials.xml for the initial step, then creates a materials.xml for the final step, and ensures that the SHA512 of each file matches. I verified the XML files look as expected, too.

drewejohnson

Thanks for the updates @gridley! I've got some comments on formatting of docstrings and testing, primarily moving from hash-based tests.

Also, can you merge in the latest develop to pick up the CI changes?

openmc/deplete/results_list.py

tests/regression_tests/deplete/test.py

drewejohnson · 2020-12-24T15:54:27Z

tests/regression_tests/deplete/test.py

+    # Check that the conversion of the final step depleted materials XML
+    # file hashes to what we expect.
+    output_xml_file = 'last_step_materials.xml'
+    last_step_materials.export_to_xml(path=output_xml_file)
+    with open(output_xml_file, 'rb') as result_file:
+        result_file_hash = hashlib.sha512()
+        for line in result_file:
+            result_file_hash.update(line)


The hash based testing is nice because we don't have to track the potentially large xml file generated by this test. But it can hide the reason why this test is failing (#1338). I'd prefer checking if the strings are identical directly. And (optionally) using difflib.unified_diff to print the difference if two files differ

Personally, I'd prefer not to make this change. Because this is a very minor piece of OpenMC, I would like to minimize the overall LOC we add to the repo in implementing this functionality.

I'd prefer we not add more hashed results if at all possible. How big is the raw data we're talking about here?

Not big, I am working on changing it to plaintext this morning.

gridley · 2021-01-03T21:07:21Z

@drewejohnson Do you definitely want me to rewrite the test to not use hashing? I really doubt this is going to fail in the future, and wouldn't even be terribly hard to debug if it does fail, which is why I initially elected to use hashing rather than a direct diff.

drewejohnson

@gridley I brought up the hash based testing because I know it has caused grief in the past. Enough that an issue was created to move away from it.

I understand your sentiment about not wanting to track in the xml file. Just not sure how to square the inclusion of a hash-based test with a desire from other developers to move away from them. But we also haven't made any movement away from them yet...

I know this PR has been up for a bit with the testing in but that was over the holiday break. I'd like to give anyone with some stronger opinions on #1338 and it's implications to this PR another day to chime in

cc @openmc-dev/technical-committee

gridley · 2021-01-04T17:18:49Z

Oh darn. I seem to have not kept up with the trend of moving away from hash-based testing. I have been frustrated deciphering hash-based test failures in the past as well. If that is a general movement with OpenMC, I am very happy to follow along.

paulromano

Thanks for taking this on @gridley! This is a really useful addition for people working on depletion simulations. A few minor comments I have:

openmc/deplete/results_list.py

paulromano · 2021-01-06T20:35:30Z

tests/regression_tests/deplete/test.py

+    # Check that the conversion of the final step depleted materials XML
+    # file hashes to what we expect.
+    output_xml_file = 'last_step_materials.xml'
+    last_step_materials.export_to_xml(path=output_xml_file)
+    with open(output_xml_file, 'rb') as result_file:
+        result_file_hash = hashlib.sha512()
+        for line in result_file:
+            result_file_hash.update(line)


I'd prefer we not add more hashed results if at all possible. How big is the raw data we're talking about here?

gridley · 2021-01-07T16:32:35Z

Alright, so to test out the file diff stuff, i just screwed up the reference file to see how this looks:


```        # Check all lines match, printing along the way
        success = True
        for line in diff_vs_expected:
            if line:
                success = False
                print(line)
>       assert success
E       assert False

/home/gavin/Code/openmc/tests/regression_tests/deplete/test.py:180: AssertionError
------------------------------------------------------------------------------------------------------------ Captured stdout call -------------------------------------------------------------------------------------------------------------
---

+++

@@ -12,7 +12,7 @@
     <nuclide ao="0.046295399999999994" name="O16" />
     <nuclide ao="0.00011276839999999999" name="O17" />
     <nuclide ao="4.611985879920533e-07" name="Cs135" />
-    <nuclide ao="9.843562041057666e-09" name="Gd156" />
+    <nuclide ao="7.843562041057666e-09" name="Gd156" />
     <nuclide ao="5.092224423114367e-10" name="Gd157" />
     <nuclide ao="1.1458035030465554e-08" name="I135" />
     <nuclide ao="1.0044107689339677e-05" name="U234" />

Any further thoughts?

gridley · 2021-01-07T16:52:20Z

OK, after a little bit more fiddling, I believe this is fully ready to go.

paulromano · 2021-01-08T20:16:38Z

@drewejohnson Are you satisfied with this PR?

drewejohnson

Thanks @gridley for working through this PR! And thanks to @rockfool for kicking it off.

Looks like GH is expecting some additional CI checks. Maybe related to #1743 ? Might have to merge in changes from the develop branch

…ials

…d test for that

Co-authored-by: Andrew Johnson <drewejohnson@users.noreply.github.com>

Co-authored-by: Paul Romano <paul.k.romano@gmail.com>

…als test

gridley · 2021-01-10T01:50:59Z

OK, rebased, let's see if CI works now...

gridley requested a review from drewejohnson as a code owner November 18, 2020 22:35

drewejohnson requested changes Nov 30, 2020

View reviewed changes

paulromano reviewed Dec 3, 2020

View reviewed changes

openmc/deplete/results_list.py Outdated Show resolved Hide resolved

drewejohnson mentioned this pull request Dec 4, 2020

Easier access and indexing to depletion time points #1732

Merged

rockfool mentioned this pull request Dec 11, 2020

Export materials.xml from depletion results #1714

Closed

gridley force-pushed the print_materials branch 2 times, most recently from e7a7e3a to a7ce070 Compare December 14, 2020 19:07

drewejohnson requested changes Dec 24, 2020

View reviewed changes

gridley force-pushed the print_materials branch from d4cfd47 to 9a01a6e Compare December 24, 2020 21:12

drewejohnson reviewed Jan 4, 2021

View reviewed changes

paulromano requested changes Jan 6, 2021

View reviewed changes

gridley changed the title ~~Print materials~~ Export of depletion results to openmc.Materials Jan 7, 2021

paulromano approved these changes Jan 8, 2021

View reviewed changes

drewejohnson approved these changes Jan 9, 2021

View reviewed changes

gridley force-pushed the print_materials branch from 5cef1e2 to 67542a3 Compare January 10, 2021 01:50

rockfool and others added 2 commits January 9, 2021 20:50

export materials.xml from depletion results

b330d33

incorporate PR feedback on writing depleted material XML files

12be239

gridley and others added 15 commits January 9, 2021 20:50

XML export of depleted material results optional, rather return Mater…

9ff35d7

…ials

incorporate review suggestions

3954b26

add more specific exceptions for ResultsList->openmc.Materials and ad…

e99ba6f

…d test for that

remove redundant code

6cf5253

Update openmc/deplete/results_list.py

63a5670

Co-authored-by: Andrew Johnson <drewejohnson@users.noreply.github.com>

Update tests/regression_tests/deplete/test.py

a2f1a2f

Co-authored-by: Andrew Johnson <drewejohnson@users.noreply.github.com>

fix rebase screwup

eec5bfc

paragrpah formatting

4d777a7

improve export_to_materials docstring

de0eef0

Co-authored-by: Paul Romano <paul.k.romano@gmail.com>

Update openmc/deplete/results_list.py

d9f94fc

Co-authored-by: Paul Romano <paul.k.romano@gmail.com>

Update openmc/deplete/results_list.py

79da586

Co-authored-by: Paul Romano <paul.k.romano@gmail.com>

use direct file diff rather than hash comparison for export_to_materi…

863f0f4

…als test

remove unnecessary newline

f018ba7

final cleanup

91bc2e5

ensure no duplicate nuclides in materials

67542a3

paulromano merged commit fa297d7 into openmc-dev:develop Jan 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export of depletion results to openmc.Materials #1726

Export of depletion results to openmc.Materials #1726

gridley commented Nov 18, 2020

drewejohnson commented Nov 25, 2020

rockfool commented Nov 25, 2020

gridley commented Nov 25, 2020

drewejohnson left a comment

gridley commented Dec 14, 2020

drewejohnson commented Dec 16, 2020

gridley commented Dec 16, 2020

drewejohnson commented Dec 16, 2020

gridley commented Dec 21, 2020

drewejohnson left a comment

drewejohnson Dec 24, 2020

gridley Dec 24, 2020

paulromano Jan 6, 2021

gridley Jan 7, 2021

gridley commented Jan 3, 2021

drewejohnson left a comment

gridley commented Jan 4, 2021 •

edited

paulromano left a comment

paulromano Jan 6, 2021

gridley commented Jan 7, 2021

gridley commented Jan 7, 2021

paulromano commented Jan 8, 2021

drewejohnson left a comment

gridley commented Jan 10, 2021

Export of depletion results to openmc.Materials #1726

Export of depletion results to openmc.Materials #1726

Conversation

gridley commented Nov 18, 2020

drewejohnson commented Nov 25, 2020

rockfool commented Nov 25, 2020

gridley commented Nov 25, 2020

drewejohnson left a comment

Choose a reason for hiding this comment

gridley commented Dec 14, 2020

drewejohnson commented Dec 16, 2020

gridley commented Dec 16, 2020

drewejohnson commented Dec 16, 2020

gridley commented Dec 21, 2020

drewejohnson left a comment

Choose a reason for hiding this comment

drewejohnson Dec 24, 2020

Choose a reason for hiding this comment

gridley Dec 24, 2020

Choose a reason for hiding this comment

paulromano Jan 6, 2021

Choose a reason for hiding this comment

gridley Jan 7, 2021

Choose a reason for hiding this comment

gridley commented Jan 3, 2021

drewejohnson left a comment

Choose a reason for hiding this comment

gridley commented Jan 4, 2021 • edited

paulromano left a comment

Choose a reason for hiding this comment

paulromano Jan 6, 2021

Choose a reason for hiding this comment

gridley commented Jan 7, 2021

gridley commented Jan 7, 2021

paulromano commented Jan 8, 2021

drewejohnson left a comment

Choose a reason for hiding this comment

gridley commented Jan 10, 2021

gridley commented Jan 4, 2021 •

edited