Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

microxs from mg flux and chain file #2755

Merged
merged 19 commits into from
Dec 22, 2023

Conversation

jbae11
Copy link
Contributor

@jbae11 jbae11 commented Nov 2, 2023

Description

This function will create a custom MicroXS object from an MG flux spectrum and chain file. It still needs a lot of work and I'm just putting it out so folks can fix it and provide advice. This is in hopes of creating a standalone activation capability, which I'm currently testing.

Lots of credit to @shimwell for the help and advice.

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)

@jbae11 jbae11 requested a review from drewejohnson as a code owner November 2, 2023 21:55
@shimwell
Copy link
Member

shimwell commented Nov 2, 2023

Congrats on your first PR @jbae11 I can tinker with this a bit and see what change be changed. Thanks for going the extra mile and converting the discussion to real code.

@shimwell
Copy link
Member

shimwell commented Nov 3, 2023

@jbae11 I've made a PR to your branch that:

  • changes the function to a class method

  • adds some tests

The part I think needs some attention is the obtaining of the cross section for the MT number at the middle energy of the energy bin. I think it might be better to convert the cross section into a multi group cross section with the MGXS class (example here) and then get the xs value of the bin from the multigroup as this will apply some rules that take into account the shape of the cross section over the bin width.

…lux_to_classmethod

Converting jin from multi group flux to classmethod
@jbae11
Copy link
Contributor Author

jbae11 commented Nov 3, 2023

Thanks @shimwell ! I merged your edits. For the MGXS, I do think there's a better way than sampling at the midpoint - I'm not too clear on how to implement the MGXS, but if you have a good implementation feel free to make another MR :)

One thing I'm keeping in mind is the speed at which this happens - ~3 minutes for an activation calculations is too long in my mind, so that's why I'm trying to keep everything a np array. Also found out that there are different [interpolation schemes available for the xs extraction] (https://github.com/openmc-dev/openmc/blob/develop/openmc/data/function.py#L184), so that's probably something we'll have to mess around once we get to validating.

@shimwell
Copy link
Member

shimwell commented Nov 3, 2023

I shall look into the MGXS a bit more but I think it can help with bringing the time down as if I recall correctly we can make the cross sections in the desired group structure for the desired nuclides and export it to a h5 file ready for reuse. Now I'm wondering if we should also distribute multigroup cross sections in h5 format for a few popular multigroup formats

Copy link
Contributor

@yardasol yardasol left a comment

Choose a reason for hiding this comment

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

Hi @jbae11,
Thanks for this PR, I think this will be a good addition. I think we should have @paulromano take a look at this since he retrofitted the MicroXS class to support MGXS.

My other comments are below

openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
Comment on lines 264 to 266
if 'fission' in reactions:
# send to back
reactions.append(reactions.pop(reactions.index('fission')))
Copy link
Contributor

Choose a reason for hiding this comment

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

Purpose for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably not the best way to implent this, but since fission is not in the reactions variable, I'm adding it later but making sure that the order stays consistent by pushing fission (mt=18) at the end for both the reactions and mt variable.

openmc/deplete/microxs.py Outdated Show resolved Hide resolved
@jbae11
Copy link
Contributor Author

jbae11 commented Nov 15, 2023

Currently, it takes a couple of minutes for this function to run, which I don't think is acceptable, given activation calculations are in the matter of seconds, is there a way to accelerate this?

@shimwell
Copy link
Member

Currently, it takes a couple of minutes for this function to run, which I don't think is acceptable, given activation calculations are in the matter of seconds, is there a way to accelerate this?

I took a look at the openmc.lib.nuclide.collapse_rate to see if this could help speed up the code but got stuck here

@paulromano
Copy link
Contributor

@jbae11 Thanks for the PR and so sorry I haven't had a chance to take a look until now! I'm glad that @shimwell brought up the idea of using collapse_rate because that's exactly the idea that I had in mind for such a functionality. I just commented on #2781 with a suggestion how to get it working, so I'll let you two take it from here but I think in general that is the right direction to take this PR. I think the workflow would look something like:

  • Get the list of nuclides you need to get reaction rates for
  • In a temporary directory, create a trivial OpenMC model with a single material containing all nuclides
  • Call openmc.lib.collapse_rate for each nuclide/reaction with the appropriate flux spectrum (whole block surrounded by openmc.lib.init and openmc.lib.finalize)

@shimwell
Copy link
Member

shimwell commented Nov 29, 2023

@jbae11 I've had a go at completing the suggestions from @paulromano and made a PR against your fork jbae11#2 for your consideration, it is significantly faster than the current approach.

openmc/deplete/microxs.py Outdated Show resolved Hide resolved
@shimwell
Copy link
Member

I've made an example python script that does a depletion simulation with a pre calculated flux spectrum.

It does take a while to make a MicroXS file for all the isotopes found in a large chain file that also appear in the cross sections file.

So I'm thinking this new class method could benefit from some more user control over how many nuclides are loaded up. Perhaps a chain_reduce argument can be added or the user could even be asked to specify the nuclides directly.

@shimwell
Copy link
Member

shimwell commented Dec 2, 2023

Thinking a bit more about ways to speed up the micro generation and taking inspiration from the get_microxs_and_flux method I plan to add a nuclides argument.

nuclides (list of str) – Nuclides to get cross sections for. If not specified, all burnable nuclides from the depletion chain file are used.

openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
@shimwell
Copy link
Member

shimwell commented Dec 5, 2023

Sorry @jbae11 I've made another PR against your branch to fix some mistakes I introduced 😳

This latest version allows us to optionally limit the micro_xs to user specified nuclides. This will produce a micro_xs quickly compared to the nuclides from the entire chain file (however this remains an option as well)

I have added another test to show how the nuclides option works

I have also updated the full ALARA/ORIGEN/FISPACT style example to use the nuclides in the material instead of the whole chain file. Example can be found here

*updated example

@jbae11 jbae11 requested a review from paulromano as a code owner December 5, 2023 13:55
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
openmc/deplete/microxs.py Outdated Show resolved Hide resolved
@jbae11
Copy link
Contributor Author

jbae11 commented Dec 14, 2023

@yardasol have we adequately addressed your comments?

@paulromano
Copy link
Contributor

@jbae11 @shimwell I've made some direct updates on this branch:

  • Changed name to from_multigroup_flux
  • Simplified logic in from_multigroup_flux, notably with how MT values are determined
  • Adding an optional reactions argument that allows a user to specify which reactions they want

Let me know if either of you have any objections to these changes. If not, I think this should be good to merge!

Copy link
Contributor

@yardasol yardasol left a comment

Choose a reason for hiding this comment

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

Hi @jbae11, these changes look good! I just have one question I'd like answered, it is below.

openmc/deplete/microxs.py Show resolved Hide resolved
@shimwell
Copy link
Member

I think @jbae11 has a few comparison simulations that might be good to run. @jbae11 is it easy to run your activator code and make these graphs again?

@jbae11
Copy link
Contributor Author

jbae11 commented Dec 21, 2023

Thanks @yardasol @shimwell @paulromano !

@paulromano - I agree with your normalization comment. Also thanks for the great edits!
@yardasol Let me know if this completes your review.
@shimwell those graphs look the same, and the discrepancy is not from the microxs, but from the operator/integrator, since the microxs values correspond with SCALE/COUPLE (SCALE's collapsed microxs generating code) values.

@paulromano
Copy link
Contributor

Ok, after thinking about the normalization more, dividing by the sum is fine as long as it is assumed that the flux values are absolute (not per eV). I added a test confirming that it works as intended.

@shimwell
Copy link
Member

Thanks for the extra test and improvements @paulromano

Thanks @jbae11 for confirming that Microxs is good and results differ elsewhere (perhaps operator/integrator). I think we should keep working on that seperatly to this PR

This PR looks good to me, it does what it set out to do. I see it has an approval already let's merge this tomorrow unless there are any objections?

@shimwell shimwell added the Merging Soon PR will be merged in < 24 hrs if no further comments are made. label Dec 21, 2023
@paulromano paulromano merged commit f590029 into openmc-dev:develop Dec 22, 2023
18 checks passed
church89 pushed a commit to openmsr/openmc that referenced this pull request Jul 18, 2024
Co-authored-by: Jin Whan Bae <[email protected]>
Co-authored-by: shimwell <[email protected]>
Co-authored-by: Jonathan Shimwell <[email protected]>
Co-authored-by: Olek <[email protected]>
Co-authored-by: Paul Romano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merging Soon PR will be merged in < 24 hrs if no further comments are made.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants