Skip to content

Conversation

@pshriwise
Copy link
Collaborator

@pshriwise pshriwise commented Jul 8, 2024

This adds a UserObject for editing OpenMC Tally and some Filter objects. The order of execution in OpenMCProblemBase should ensure they can be used in conjunction to create custom, controllable tallies in a Cardinal input file (for the set of filters supported, see below).

OpenMCTallyEditor

This UO can be used to either edit an existing tally object or create a new one via the Cardinal input file. A tally's scores, nuclides, filters (set by ID) can all be specified in the input file. These parameters can also be controlled using the MOOSE control system. The tally's multiply_density attribute, governing whether or not tally results are multiplied by atom density, can be set as well, but as this is used less often it is set to false by default. multiply_density can also be controlled via the MOOSE control system. Other parameters are required.

If the create_tally parameter of an OpenMCTallyEditor object is true and a tally already exists with the specified ID, a warning is shown. If create_tally is false and a tally with the specified ID doesn't exist, an error is raised.

The tally UO provides functionality that can replace the OpenMCTallyNuclides and I've done so in this PR. I'm not sure that deprecation of the existing OpenMCTallyNuclides object is necessary given its short lifetime, but I'm happy to provide that here.

OpenMCDomainFilterEditor

The domain filter editor allows for the creation of tally filters and modification of their associated bins (by cell/material/universe/mesh ID). Like the tally properties above, filter bins can be set in the input file and modified between OpenMC executions as part of the MOOSE control system.

The supported filter types for this UO include: CellFilter, MaterialFilter, UniverseFilter, and MeshFilter. These are all spatial/domain filters that have the same generic bin structure and are important for depletion/activation workflows that may be attached to Cardinal multiphysics problems.

The filter editor UO is complicated by the necessity of a filter type. Additional checks have been added that ensure:

  • if create_filter is true and a tally filter with the specified ID already exists:
    • when filter_type parameter is set, the specified filter_type is checked against the existing Filter's type and an error is raised on mismatch
    • if filter_type is not set or the types match, a warning is shown
  • if create_filter is false and a filter with a matching ID exists:
    • when filter_type parameter is set, the specified filter_type is checked against the existing Filter's type and an error is raised on mismatch
      • if filter_type is not set, no additional check is performed other than to check that the filter is in the set of types supported by OpenMCDomainFilter

TODO:

  • Complete TallyEditor testing
  • Complete DomainFilterEditor testing
  • tally editor docs
  • domain filter editor docs

Closes #837

@pshriwise pshriwise changed the title Tally uos Tally and Domain Filter Editor UOs Jul 8, 2024
@moosebuild
Copy link
Collaborator

moosebuild commented Jul 8, 2024

Job Documentation on 107d06f wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Collaborator

moosebuild commented Jul 8, 2024

Job Coverage on 107d06f wanted to post the following:

Coverage

5a5190 #923 107d06
Total Total +/- New
Rate 93.39% 93.32% -0.08% 90.91%
Hits 7436 7594 +158 200
Misses 526 544 +18 20

Diff coverage report

Full coverage report

This comment will be updated on new commits.

Copy link
Contributor

@nuclearkevin nuclearkevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After going over the changes, I don't think there are any clashes between these userobjects and the tally system when xml tallies/filters are modified. I also don't believe this will cause any issues with the filter system coming in #951 since those are thin wrappers that return values stored in OpenMC filter objects.

I can see issues arising if the user attempts to modify tallies/filters added through the [Problem/Tallies] block since the changes made by these userobjects don't get propagated to either CellTally or MeshTally wrapper classes. It might be worth restricting the use of both userobjects so they only modify xml tallies or create new ones, unless you want to find a way to propagate changes in the filter/tallies to CellTally and MeshTally.

@pshriwise
Copy link
Collaborator Author

@aprilnovak I'm seeing similar Nek-related test failures on other PRs and the changes here don't seem relevant (correct me if I'm wront) so I'm not going to dig into that right away.

@aprilnovak
Copy link
Collaborator

You can ignore the container failures -- the Nek ones are all passing, the JIT seems to have gotten slower so there's just some timeouts but they are fine.

@loganharbour, anything special we should be doing about the container failures? I believe this is also related to the failure on #952

@moosebuild
Copy link
Collaborator

Job Precheck on b25c149 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/923/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 5a5190e6b3e40f7c5f9f0288b0055ec38e0d1c6b

@pshriwise
Copy link
Collaborator Author

@nuclearkevin @aprilnovak I believe I've addressed all outstanding issues at this point. I've added additional checks and tests that the tallies with editors aren't tallies generated by Cardinal for data mapping.

I started to add this for filters as well, but I realized that it isn't really possible for filters to overlap because the filters currently supported by the tally editors are currently in the set of XML filter types that are not allowed for application with Cardinal tallies, so I think we're okay there. I'm happy to add the code to double-check this if it would make everyone more comfortable, but it wouldn't be straightforward to test really. I'm thinking that it's something to note for later. (I do have a proposal on how we might be able to unify these features with the nice work Kevin has done on the current tally system as a follow-on).

@aprilnovak @loganharbour I'm not sure why CI isn't running on this PR. It seems it isn't happy with the SHA (not the one in the git history though I'll note). Thoughts?

@aprilnovak
Copy link
Collaborator

Thanks @pshriwise! I'll give this another round of review today.

The CI isn't running because there's a merge conflict on the OpenMCTallyNuclides.md file. If you fix it and push up a change, the tests should launch.

@nuclearkevin
Copy link
Contributor

@pshriwise Thanks for addressing my concerns! I'll go over the changes again later today.

@aprilnovak
Copy link
Collaborator

I added a commit which makes OpenMCUserObject the base class for all the OpenMC-related userobjects (2 new ones in this PR, but also some pre-existing user objects). This allowed me to remove two of the tests being added in this PR, since now we can use a single test to check the wrong_problem.i catch (that user does not try to apply these OpenMC-only objects without OpenMCCellAverageProblem).

@moosebuild
Copy link
Collaborator

Job Precheck, step Clang format on a745bbe 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/923/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 ae4770d3ea774e12bf0b8558497d94fd4af45291

@aprilnovak
Copy link
Collaborator

Is this ready to go?

@pshriwise
Copy link
Collaborator Author

As far as I'm concerned yep

@aprilnovak
Copy link
Collaborator

LGTM!

@aprilnovak aprilnovak merged commit f50dc82 into neams-th-coe:devel Jan 23, 2025
9 of 10 checks passed
@aprilnovak aprilnovak mentioned this pull request Jan 27, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow control of OpenMC depletion tallies

4 participants