neams-th-coe / cardinal

High-Fidelity Multiphysics
https://cardinal.cels.anl.gov/
Other
91 stars 45 forks source link

Tally energy filters #922

Closed nuclearkevin closed 3 months ago

nuclearkevin commented 3 months ago

This PR adds energy binning to tallies in Cardinal. The main motivation is to enable multiphysics coupling to applications that require multi-group fluxes (e.g. Bison, which requires a fast flux) and tally post-processing for comparisons with deterministic reactor physics applications (Griffin, Moltres, etc.). Down the line it could also enable multi-group cross section generation with Cardinal if that's of interest.

At the moment energy binning does not differentiate between different scores. This becomes somewhat clunky for many groups as you need to sum scores over energy bins manually in the input file when a total source is required. My thoughts are that this could be mitigated by adding an AuxKernel to do the summation automatically, or allowing users to specify scores that they do not want binned by energy and create separate tally objects for those scores. Any input on the most desirable solution to this user inconvenience would be appreciated.

Closes #602

moosebuild commented 3 months ago

Job Precheck on 1650681 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/cardinal/docs/PRs/922/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 0ba4efdc6ff4b54ffb7af359678410e93dd0c75e

moosebuild commented 3 months ago

Job Documentation on 1e78585 wanted to post the following:

View the site here

This comment will be updated on new commits.

aprilnovak commented 3 months ago

Awesome!! This has been towards the top of the user feature request list for a very long time.

Part of the reason why I didn't tackle it yet is due to some of the complexities you noted -- and I've been wondering if it's time we do a refactor of Cardinal's tallies to extricate it from the OpenMCCellAverageProblem class. Limitations of the current setup include:

I'm thinking it may be nice to invest in a custom action syntax. A very rough cut of what I'm envisioning:

[Tallies]
  [t1]
    type = CellTally
    scores = 'kappa_fission heating'
    blocks = '1 2'
  []
  [t2]
    type = MeshTally
    scores = 'flux
    energy_filter = '0.0 5.0 20e6'
    trigger = 'rel_err'
    trigger_threshold = 1e-2
  []
[]
nuclearkevin commented 3 months ago

@aprilnovak Removing tallies from the OpenMCCellAverageProblem class and abstracting them into their own system with Actions sounds like a fantastic solution - it beats either of my hacky workarounds. With a change that size it may be worth keeping the old OpenMCCellAverageProblem around (possibly renamed to LegacyOpenMCCellAverageProblem) with a deprecation warning and date to avoid breaking existing models.

If nobody else is working on these changes I can rough out an implementation over the next week or so (assuming I don't get too busy with TRISO work here at McMaster). In the meantime I'll close this PR as it'll become outdated with the new system.

aprilnovak commented 3 months ago

Sounds great!

I'd be in favor of just applying the changes directly to OpenMCCellAverageProblem -- if we were to rename that class to Legacy... then users would still need to update their files (so IMO it doesn't fix 100% backwards compatibility desires).

Maybe to ensure 100% backwards compatibility, we can add a temporary boolean like use_new_tally_system which defaults to False. If a user sets this to true, we bypass all the tally logic in OpenMCCellAverageProblem in favor of the Action approach. But maybe this is overly complicated and we just bite the bullet and ask users to update.

moosebuild commented 3 months ago

Job Coverage on 1e78585 wanted to post the following:

Coverage

0ba4ef #922 1e7858
Total Total +/- New
Rate 93.55% 93.45% -0.10% 87.65%
Hits 7265 7309 +44 71
Misses 501 512 +11 10

Diff coverage report

Full coverage report

Warnings

This comment will be updated on new commits.