Adding methods to automatically apply results to existing Tally objects. #2671

pshriwise · 2023-08-28T19:11:40Z

Description

One thing I've always found a bit clunky when working with our simulation results is the process of extracting user-specified tally results from the statepoint file. It seems to me that finding a match is left to the user, but it should be possible to automate that in the majority of cases.

The idea here is that we can now apply results from our statepoint file to the original tally objects created as inputs to the simulation. This simplifies situations like:

tally = openmc.Tally()
model.tallies = [tally]
# further tally setup here

sp_file = model.run()
with openmc.StatePoint(sp_file) as sp:
    sp_tally = sp.get_tally(id=tally.id)

# do result postprocessing here with the new sp_tally object, two objects to keep track of

to

tally = openmc.Tally()
model.tallies = [tally]
# further tally setup here

sp_file = model.run()
tally.add_results(sp_file)

# only one tally object to worry about 
# do result postprocessing here with original tally object

or even

tally = openmc.Tally()
model.tallies = [tally]
# further tally setup here

sp_file = model.run(apply_tally_results=True)

# do result postprocessing here with original tally

Keeping this as a draft for now as I'm kind of workshopping it. But I'll take care of the following dev tasks soon if there aren't any objections to heading this direction.

I have performed a self-review of my own code
I have followed the style guidelines for Python source files (if applicable)
I have made corresponding changes to the documentation (if applicable)
I have added tests that prove my fix is effective or that my feature works (if applicable)

MicahGale

I like this feature. My one concern is that right now this PR drops coverage.

openmc/statepoint.py

pshriwise · 2024-09-20T14:51:23Z

After a good amount of massaging related to the Tally.estimator and Tally object equivalence I came to the following conclusion:

When the Tally.estimator attribute is None this is effectively an indication that OpenMC should select an appropriate estimator for the filters and tallies applied. (This is often the case from user tally generation code I've seen). At runtime, OpenMC will select and apply an appropriate estimator with a preference for the tracklength estimator if allowed. This estimator is written to the state point file explicitly, as it should be, but it makes matching the original, "input" Tally object to the Tally object in the StatePoint object less straightforward.

I've updated the documentation about the Tally.estimator attribute to reflect this understanding and the Tally.__eq__ method allows a match between tallies if either of the Tally.estimator attributes are set to None.

With that, I think I'm pretty happy with the state of this pending further review or something I've missed (impossible); I'll be looking forward to updating tutorials/notebooks to use this feature as I think it will make setup and post-processing in a single script/notebook more intuitive.

@MicahGale @paulromano @jtramm I'd be curious to hear your thoughts in particular!

…ifferent than an explicity specified estimator

…n Tally.__eq__. The default value of None for the `Tally.estimator` attribute allows the C++ code to provide additional warnings/errors if a user explicitly specifies an estimator that is incompatible with the tally filters/scores. When this value is None, no entry appears in the XML inputs and OpenMC is free to choose an appropriate estimator without error.

…lence check

…attribute

MicahGale · 2024-10-16T16:25:02Z

I still need to pull this branch and play around with it a bit. My first inclination for the equality of None is: why use implicit defaults? If an explicit default were you used where it just defaulted to track length, then all the __eq__ logic would be unnecessary, and would be overall more intuitive for the users.

pshriwise · 2024-10-16T17:45:20Z

I still need to pull this branch and play around with it a bit. My first inclination for the equality of None is: why use implicit defaults? If an explicit default were you used where it just defaulted to track length, then all the __eq__ logic would be unnecessary, and would be overall more intuitive for the users.

Yeah I agree that an explicit default might be more clear in terms of intention, but I think the equivalence logic would mainly remain the same. We can't really rely on 'tracklength' as the default value because there are restrictions for estimators on certain scores/filters, but the C++ will try to respect an explicitly set estimator on the Tally object currently.

openmc/src/tallies/tally.cpp

Line 283 in dcb2557

// Check if user specified estimator

Explicitly setting tracklength is going to cause a lot of problems in the test suite and a lot of this implicit default is already baked into that in one way or another. I actually tried this out earlier in this PR but it was clear that it's sort of its own mess to untangle if we want to change that behavior.

Another option is on the C++ side, the calls to fatal_error could be reduced to warnings with an update to the applied estimator as needed, but I prefer the status quo tbh.

MicahGale · 2024-11-06T15:50:16Z

Sorry for the delay, yes that makes sense for now. Long term it would be nice it the default were automatically changed as needed, but that's beyond this scope.

MicahGale · 2024-11-06T15:51:04Z

openmc/model/model.py

@@ -644,6 +645,11 @@ def run(self, particles=None, threads=None, geometry_debug=False,
            to True.

            .. versionadded:: 0.13.3
+        apply_tally_results : bool
+            Whether or not to apply results of the final statepoint file to the


@tjlaboss would say:

Suggested change

Whether or not to apply results of the final statepoint file to the

Whether to apply results of the final statepoint file to the

model's tally objects.

MicahGale · 2024-11-06T15:52:35Z

openmc/statepoint.py

-                    group = tallies_group[f'tally {tally_id}']
-
-                    # Check if tally is internal and therefore has no data


This seems to have moved to _populate_tally, but it doesn't seem like that's always ran. Is that ok that this won't always be ran?

MicahGale · 2024-11-06T15:56:35Z

openmc/statepoint.py

+        sp_tally = self.get_tally(tally.scores,
+                                    tally.filters,
+                                    tally.nuclides,
+                                    tally.name,
+                                    tally.id,
+                                    tally.estimator,
+                                    exact_filters=True,
+                                    exact_nuclides=True,
+                                    exact_scores=True,
+                                    multiply_density=True,
+                                    derivative=tally.derivative)


This doesn't feel PEP8. With such a long arguments list keyword arguments should be used to avoid off-by-1 errors.

MicahGale · 2024-11-06T15:57:47Z

openmc/statepoint.py

@@ -564,6 +611,12 @@ def get_tally(self, scores=[], filters=[], nuclides=[],
            If True, the number of scores in the parameters must be identical
            to those in the matching Tally. If False (default), the scores
            in the parameters may be a subset of those in the matching Tally.
+            Default is None (no check).
+        multiply_density : bool, optional


This feels more like an enforce_multipl_dens than a multiply_density

MicahGale · 2024-11-06T16:01:37Z

openmc/statepoint.py

@@ -591,17 +644,25 @@ def get_tally(self, scores=[], filters=[], nuclides=[],
            if id and id != test_tally.id:
                continue

-            # Determine if Tally has queried estimator


This is a bit out of scope, but why iterate over self.tallies.values()? This seems like its a dictionary, and doing it this makes it an O(N^2) operation to find all tallies.

MicahGale · 2024-11-06T16:02:06Z

openmc/tallies.py

@@ -33,7 +33,7 @@
 _FILTER_CLASSES = (openmc.Filter, openmc.CrossFilter, openmc.AggregateFilter)

 # Valid types of estimators
-ESTIMATOR_TYPES = ['tracklength', 'collision', 'analog']
+ESTIMATOR_TYPES = ('tracklength', 'collision', 'analog')


Might as well just do a set for slightly better performance.

MicahGale · 2024-11-06T16:04:43Z

openmc/tallies.py

+            other_nuclides.remove('total')
+        if 'total' in self_nuclides:
+            self_nuclides.remove('total')
+        if other_nuclides != self_nuclides:


What dataset is nuclides? Does this require proper sorting? Maybe sets?

MicahGale · 2024-11-06T16:05:58Z

openmc/tallies.py

+            self_nuclides.remove('total')
+        if other_nuclides != self_nuclides:
+            return False
+        if other.scores != self.scores:


A lot of this is repeated code.

Is

for attr_name in {"scores", "triggers"...}: if getattr(other, attr_name) != getattr(self, attr_name): return False

too clever?

pshriwise marked this pull request as draft August 28, 2023 19:15

pshriwise force-pushed the apply_tally_results branch from 02d7f3d to 1838375 Compare August 29, 2023 03:35

pshriwise force-pushed the apply_tally_results branch from 1838375 to f72b96f Compare April 3, 2024 04:31

pshriwise force-pushed the apply_tally_results branch from f72b96f to 54c938e Compare April 10, 2024 08:24

pshriwise marked this pull request as ready for review April 10, 2024 09:22

MicahGale reviewed Apr 10, 2024

View reviewed changes

openmc/statepoint.py Outdated Show resolved Hide resolved

openmc/statepoint.py Outdated Show resolved Hide resolved

MicahGale mentioned this pull request Apr 11, 2024

Allow getting cells, surfaces, materials by ID in constant-ish time #2953

Open

pshriwise force-pushed the apply_tally_results branch from bc86858 to 5e249a9 Compare April 30, 2024 20:15

pshriwise force-pushed the apply_tally_results branch from 0690457 to 25dd302 Compare September 13, 2024 21:30

pshriwise self-assigned this Sep 20, 2024

pshriwise added 19 commits October 16, 2024 10:44

Adding method to automatically apply tally results to existing objects.

a364244

Slight refactor and allowing application through Model.run

481dd48

properly handle internal tallies

d58c811

Some slight cleanup

6322a30

Adding test

0f9d35c

Replacing filters, nuclides, scores instead of appending.

a761b42

Updates to tally equivalence and version numbers

41841f1

Updates to the tally class to account for estimator type

495eec2

Update version in add_results docstring

df3469a

Moving additional checks into the Statepoint.get_tally method

5734b75

Adding tmate debug

165b340

Adopt the tally estimator from the statepoint file and warn if it's d…

0f69c6f

…ifferent than an explicity specified estimator

Adding a comment to explain flexibility of tally estimators in equiva…

e8fcebf

…lence check

Allow estimator to be set to None

d39c4a5

Propoagate passive None value to StatePoint.get_tally

38b1467

Update expected test result basd on conclusion about tally estimator …

5af1eea

…attribute

Explicitly allow setting tally estimator to None

5e46acf

Correction to object name in get_tally condition for estimator match

fa4d976

pshriwise added 4 commits October 16, 2024 10:45

Eesh. Correction to control flow of Tally.estimator property setter

03d80eb

Set estimator regardless of warning

7699d48

A little bit of self-review

3ac0ca3

Removing tmate from CI

2fb7e3d

pshriwise force-pushed the apply_tally_results branch from 8466fb4 to 2fb7e3d Compare October 16, 2024 15:46

MicahGale reviewed Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding methods to automatically apply results to existing Tally objects. #2671

Adding methods to automatically apply results to existing Tally objects. #2671

pshriwise commented Aug 28, 2023 •

edited

Loading

MicahGale left a comment

pshriwise commented Sep 20, 2024

MicahGale commented Oct 16, 2024

pshriwise commented Oct 16, 2024 •

edited

Loading

MicahGale commented Nov 6, 2024

MicahGale Nov 6, 2024

MicahGale Nov 6, 2024

MicahGale Nov 6, 2024

MicahGale Nov 6, 2024

MicahGale Nov 6, 2024

MicahGale Nov 6, 2024

MicahGale Nov 6, 2024

MicahGale Nov 6, 2024

	Whether or not to apply results of the final statepoint file to the
	Whether to apply results of the final statepoint file to the
	model's tally objects.

		group = tallies_group[f'tally {tally_id}']

		# Check if tally is internal and therefore has no data

Adding methods to automatically apply results to existing Tally objects. #2671

Are you sure you want to change the base?

Adding methods to automatically apply results to existing Tally objects. #2671

Conversation

pshriwise commented Aug 28, 2023 • edited Loading

Description

MicahGale left a comment

Choose a reason for hiding this comment

pshriwise commented Sep 20, 2024

MicahGale commented Oct 16, 2024

pshriwise commented Oct 16, 2024 • edited Loading

MicahGale commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise commented Aug 28, 2023 •

edited

Loading

pshriwise commented Oct 16, 2024 •

edited

Loading