Skip to content

Conversation

shimwell
Copy link
Member

Description

This PR changes the behaviour of material.add_nuclide so that if a nuclide with the same name and percentage type exists in the material then the new nuclide will be combined with the existing one.

If the nuclide exists in the material but has a different percentage type then a warning message will be printed to the user.

I think this PR puts us in better place than currently as it solves the issue of adding like nuclides in some cases and warns the user in other cases. Previously the nuclides were silently duplicated.

As discussed on the forum
https://openmc.discourse.group/t/question-about-percent-type-argument-of-the-new-method-add-elements-from-formula-introduced-in-version-0-12/702/5

Fixes # (issue)

Checklist

  • I have performed a self-review of my own code
  • 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)

@makeclean
Copy link
Contributor

Not suggesting that this should be a blocker, but I've heard a number of times from analysts they often like to see repeated nuclides when they come from different mixtures, i.e. keeping the contributions from different nucldies printed in blocks by mixtures, repeating nuclides as needed. I can see it both ways, you have a clear record in the python file which made your problem, so thats tracable, alhtough you might keep the xml file left behind. So might it be worth haveing a flag which gives the option to squash as an argument to that function, which defaults to true?

@shimwell
Copy link
Member Author

shimwell commented Sep 11, 2025

analysts they often like to see repeated nuclides when they come from different mixtures, i.e. keeping the contributions from different nucldies printed in blocks by mixtures, repeating nuclides as needed.

ok fair enough,

optional squashing (default true) when exporting to xml sounds like a more suitable solution then

Copy link
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.

I can understand both arguments here but I do lean toward the opinion that materials should not have nuclides duplicated. It may happen to work OK on the C++ side right now but it's not hard to imagine some implicit assumption of the nuclides being unique being introduced in the future.

Separate from the considerations already discussed, one thing I don't like here is that this makes building a material O(N²) since each time a nuclide is added it loops over the entire list of nuclides. The obvious way around that is to store a dictionary per material but that also means more memory usage, which I'm not crazy about 🤔

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.

3 participants