Skip to content

Fixed a bug in charged particle energy deposition.#3416

Merged
paulromano merged 13 commits intoopenmc-dev:developfrom
GuySten:charged_per_nuclide_heating
Jun 3, 2025
Merged

Fixed a bug in charged particle energy deposition.#3416
paulromano merged 13 commits intoopenmc-dev:developfrom
GuySten:charged_per_nuclide_heating

Conversation

@GuySten
Copy link
Copy Markdown
Contributor

@GuySten GuySten commented May 23, 2025

Description

Currently, OpenMC deposit charged particle energy without specifying event_nuclide.
This means that when calculating heating per nuclide and total heating the sum of heating per nuclide is not equal the total heating. This pull request divide the energy deposited by charged particles in a material to be proportional to the fractional nuclide charge density.
.
Fixes #3317

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • 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)

@GuySten GuySten requested a review from amandalund as a code owner May 23, 2025 00:28
@MicahGale
Copy link
Copy Markdown
Contributor

For a change of this kind to how tallies function I think this should also be documented in the theory manual.

Along those lines I'm having trouble with understanding how these tallies should be behaving? Could you write up the math of what is happening with this change? Possibly including the theory from the HEATR module of module of NJOY so we can see how this tally derives from the "base" nuclear data?

@GuySten
Copy link
Copy Markdown
Contributor Author

GuySten commented May 25, 2025

For a change of this kind to how tallies function I think this should also be documented in the theory manual.

Along those lines I'm having trouble with understanding how these tallies should be behaving? Could you write up the math of what is happening with this change? Possibly including the theory from the HEATR module of module of NJOY so we can see how this tally derives from the "base" nuclear data?

I have improved the documentation of energy deposition from photons and charged particles in general and this feature specifically. I also refactored the logic for scoring heat in general to avoid code repitition.

Now this pull request is ready for review.

@shimwell
Copy link
Copy Markdown
Member

I've not looked through the PR in much detail, I've just run the script in issue #3317 and I think this PR doesn't change the situation of individual heating not adding up to total heating. I will double check I'm running the
Screenshot From 2025-05-28 19-21-11

@GuySten
Copy link
Copy Markdown
Contributor Author

GuySten commented May 28, 2025

I've not looked through the PR in much detail, I've just run the script in issue #3317 and I think this PR doesn't change the situation of individual heating not adding up to total heating. I will double check I'm running the Screenshot From 2025-05-28 19-21-11

That's weird. I changed the code so that this problem does not appear in a test that contain a simpler incident photon source. I think I will add your original minimal working example as another regression test and I will try to make it work. Maybe there is another problem in heating calculations.

@GuySten GuySten marked this pull request as draft May 28, 2025 18:36
@shimwell
Copy link
Copy Markdown
Member

Might be user error on my side. I shall try again for a docker container soon

@GuySten GuySten marked this pull request as ready for review May 28, 2025 19:57
@GuySten
Copy link
Copy Markdown
Contributor Author

GuySten commented May 28, 2025

Might be user error on my side. I shall try again for a docker container soon

I suspect you have some user error because when I run your script I don't see any problem.
image

Comment thread docs/source/methods/energy_deposition.rst Outdated
Comment thread docs/source/methods/energy_deposition.rst Outdated
@shimwell
Copy link
Copy Markdown
Member

This is fab it certainly does fix the error, for some reason I had compiled your tff branch instead of this one. Sorry my my mistake there. This does fix the issue I raised.

By the way thanks for all the PRs fixing bugs, really useful work.

Sorry for the slower than typical reviews the OpenMC conference is on at the moment and I think we will be back to normal next week.

I see you have

Let us try to get this merged early next week once the conference is finished

Co-authored-by: Jonathan Shimwell <drshimwell@gmail.com>
Copy link
Copy Markdown
Member

@shimwell shimwell left a comment

Choose a reason for hiding this comment

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

Many thanks for the fix,
Will merge on Tuesday if there are no objections

Copy link
Copy Markdown
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Just putting a placeholder review -- I'm making some updates on this branch and should have those in shortly.

Copy link
Copy Markdown
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks for this fix @GuySten! I made a few updates on this branch:

  • Precomputed the total charge density so that it doesn't have to be computed each time a tally is made
  • Use mat_nuclide_index_ to get the index of a nuclide for the atom density vector without a linear search
  • Changed the regression test to a unit test where the sum of the by-nuclide heating is checked against the total.
  • Updated documentation

@paulromano paulromano enabled auto-merge (squash) June 3, 2025 13:09
auto-merge was automatically disabled June 3, 2025 14:25

Head branch was pushed to by a user without write access

@GuySten
Copy link
Copy Markdown
Contributor Author

GuySten commented Jun 3, 2025

I've added a fix to stop openmc from crashing in mg mode. waiting for the tests to pass.

One more fix was needed...

@paulromano paulromano enabled auto-merge (squash) June 3, 2025 15:59
@paulromano paulromano merged commit ace73ab into openmc-dev:develop Jun 3, 2025
14 checks passed
@GuySten GuySten deleted the charged_per_nuclide_heating branch June 3, 2025 18:09
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.

Heating tally and nuclides does not match tally without nuclides

4 participants