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

Add OpenMC label to ThermalScattering #1792

Conversation

AidanMcDonald
Copy link
Contributor

What is the change?

Added support for thermal scattering data labels in OpenMC's syntax (usually something like "c_H_in_H2O") analogous to current support for ENDFB8 and ACE labels. Also added tests similar to the existing ENDFB8 and ACE label tests. The label generator does not currently work for all thermal scattering data available to OpenMC (for example "c_O_in_H2O_solid" or "c_ortho_H"), but it works for all thermal scattering laws used in ARMI in thermalScattering.byNbAndCompound.

Why is the change being made?

An OpenMC label generator is necessary to use thermal scattering data in the ARMI OpenMC plugin, and it is simpler to include it in ARMI's thermal scattering module rather than in the plugin itself.


Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

@john-science john-science self-requested a review July 23, 2024 02:57
@john-science john-science added the feature request Smaller user request label Jul 23, 2024
@jakehader
Copy link
Member

Hi @AidanMcDonald - would it be possible to implement this in the OpenMC plugin directly or can we think about ways to abstract the thermal scattering laws implementation so that specific code-based names and attributes aren't added to the code base? I think this would be a more robust way of handling the framework for "n" codes and so that if OpenMC changes the naming it doesn't impact the framework design.

Copy link
Member

@jakehader jakehader left a comment

Choose a reason for hiding this comment

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

See general comment above. I see your comment on it being easier to implement in the framework, but maybe we can make the framework easier to subclass.

@AidanMcDonald
Copy link
Contributor Author

@jakehader , thanks for the feedback. Here's my reasoning for implementing it this way:

My intention is to implement the OpenMC label generator in the same way as the current ENDFB8 and ACE label generators since the generation and use is almost identical. I agree that adding specific names isn't ideal, but just like with the ENDF and ACE label generators, some of the labels don't follow logical rules and must be hard-coded. Since that is the case, I think it makes sense to keep the hard-coded ENDF, ACE, and OpenMC labels together like the hard-coded ENDF and ACE labels are now.

With that said, I have made a function in the OpenMC plugin that will work for now (featuring a lot of if statements for the "hard-coded" labels) if that's the direction you want to go.

@jakehader jakehader marked this pull request as ready for review July 26, 2024 02:09
@jakehader jakehader marked this pull request as draft July 26, 2024 02:09
@AidanMcDonald
Copy link
Contributor Author

Moving discussion to #1804 and closing for now. I will keep the branch in my fork just in case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants