Skip to content

Approximate multigroup velocity #3766

Merged
paulromano merged 54 commits into
openmc-dev:developfrom
GuySten:check-xs-vacuum
Apr 2, 2026
Merged

Approximate multigroup velocity #3766
paulromano merged 54 commits into
openmc-dev:developfrom
GuySten:check-xs-vacuum

Conversation

@GuySten

@GuySten GuySten commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

Description

Currently, void regions in multigroup mode makes openmc crash (#3723).

This PR implements the following changes:

  1. When inverse-velocity data is missing default to an approximate formula (group collapse of inverse-velocity data with 1/E spectrum).
  2. Ensure that lowest neutron energy boundary is at least 1e-5 (ensure 1/E spectra is defined)

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 nelsonag as a code owner February 4, 2026 00:43
@GuySten
GuySten requested review from paulromano and pshriwise and removed request for nelsonag and paulromano February 4, 2026 00:43
@GuySten GuySten added the Bugs label Feb 4, 2026
@paulromano
paulromano requested a review from nelsonag February 4, 2026 03:43

@paulromano paulromano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@nelsonag would love to get your thoughts on this one!

@GuySten GuySten added the MGXS label Feb 5, 2026
@nelsonag

nelsonag commented Feb 6, 2026

Copy link
Copy Markdown
Contributor

Why not tally a 1/v based on the group midpoint energy (I assume logarithmic midpoint is best) if no specific data is present? This could be done void or not.
I agree having data is better than not, but can assume this would work great for a UFG collapse sort of approach.

@GuySten

GuySten commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Do you mean instead of defaulting to speed 0 when 1/v data is missing using velocity that match the logarithmic energy midpoint of the group?

@nelsonag

nelsonag commented Feb 6, 2026 via email

Copy link
Copy Markdown
Contributor

@GuySten
GuySten requested a review from nelsonag March 7, 2026 21:02
@GuySten

GuySten commented Mar 7, 2026

Copy link
Copy Markdown
Contributor Author

Now this PR is ready again.

@nelsonag

Copy link
Copy Markdown
Contributor

Looking good! One more thing, sorry I should have realized sooner. We should update the docs to explain what happens when inverse-velocity is not provided in an Mgxs.h5

@GuySten

GuySten commented Mar 10, 2026

Copy link
Copy Markdown
Contributor Author

@nelsonag, I've added a section in the theory manual.

Minor editorial correction to cross_sections.rst
Comment thread docs/source/methods/cross_sections.rst
@GuySten
GuySten requested a review from nelsonag March 11, 2026 00:17

@nelsonag nelsonag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

@GuySten GuySten added the Merging Soon PR will be merged in < 24 hrs if no further comments are made. label Mar 13, 2026
Comment thread docs/source/methods/cross_sections.rst Outdated
Comment on lines +318 to +321
.. math::

v_g = \frac{1}{c \log\left(\frac{E_{\text{max}}^g}{E_{\text{min}}^g}\right)}
\left[ 2(\arctan(k_\text{max}) - \arctan(k_\text{min})) + (k_\text{max}-k_\text{min})) \right]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I couldn't get this solution independently. As best as I can tell, the solution should use arctanh(1/k) instead of arctan(k). Here's the solution from ChatGPT 5.4 Thinking. Also, it's confusing to me to use k as a normalization constant above and then use k in a change or variables too.

@GuySten GuySten Mar 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, the solution should use arctanh instead of arctan.

You can find the solution here:

https://www.wolframalpha.com/input?i=integrate+%28x%2Ba%29%2F%28x*sqrt%28x*%28x%2B2*a%29%29%29+

I will update the PR.

@GuySten GuySten Mar 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've updated the formula and documentation.

I also changed the normalization constant to alpha.

@GuySten
GuySten requested a review from paulromano March 14, 2026 21:16
@GuySten

GuySten commented Mar 22, 2026

Copy link
Copy Markdown
Contributor Author

@paulromano, do you agree with my changes?

@meltawila

Copy link
Copy Markdown

@paulromano @GuySten, Looking forward to seeing this get merged for my use case. Just wanted to make sure it's still on your radar

@GuySten

GuySten commented Apr 2, 2026

Copy link
Copy Markdown
Contributor Author

I think this PR is ready to be merged. I am waiting for @paulromano to say if he agrees.

@paulromano paulromano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, I just had a chance to go through again. I made one update to avoid complex math by using the following relationship that holds for k > 1:

Re[arctanh(k)] = arctanh(1/k)

The equation in the documentation also needed a few fixes but is good to go now. Thanks @GuySten!

@paulromano
paulromano enabled auto-merge (squash) April 2, 2026 16:19
@paulromano
paulromano merged commit d9b30bb into openmc-dev:develop Apr 2, 2026
16 checks passed
@GuySten
GuySten deleted the check-xs-vacuum branch April 2, 2026 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugs Merging Soon PR will be merged in < 24 hrs if no further comments are made. MGXS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants