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

Don't validate material modifications if they are None #1369

Conversation

drewj-usnctech
Copy link
Contributor

What is the change?

Material modifiers are not validated if the corresponding value is None

Why is the change being made?

During block construction, an error can be raised if a material modifier does not apply to any components in the block. This is great, and has already caught some errors. However, if a material modifier applies to some components in other blocks, and it not applied to a block without components that support that modifier, an error is raised.

Consider two blocks in an assembly, one fuel and the other axial reflector. The enrichment could be set in the fueled block with U235_wt_frac, but setting a U235 enrichment in an the axial reflector does not make sense. The following YAML input

U235_wt_frac:
  - 0.05
  -

would support this case, but the material validation fails because no component in the axial reflector supports the U235_wt_frac modifier.

Closes #1367

Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
    • Open to talking testing
  • The release notes (location doc/release/0.X.rst) are up-to-date with any important changes.
    • Maybe not necessary?
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@john-science
Copy link
Member

Is None something we want to support?

Why not throw an error here and force people to set it to 0.0?

What is the benefit of supporting None? It isn't even a valid YAML file any more.

@john-science john-science self-requested a review August 10, 2023 15:16
@drewj-usnctech
Copy link
Contributor Author

Is None something we want to support?

Why not throw an error here and force people to set it to 0.0?

I'm inclined to agree w/ @keckler with

Forcing a zero value just doesn't really make sense when that particular modifier doesn't apply to a given material. I would say that is less desirable than a None value or empty string.

(from #1367 (comment))

There are some cases where you may want to pass a literal 0.0 as a modifier and it actually be applied. 0% enrichment isn't a great example, but maybe a mixing fraction?

What is the benefit of supporting None? It isn't even a valid YAML file any more.

It's loadable with ruamel at least (see #1367 (comment))

I will close by saying this is probably something that should first be discussed as a proposed change in #1367: supporting either empty strings and/or None as a signal that a modifier should not be applied. In the general trend of there should be one way to do things, and current ARMI inertia for relying on empty strings for this capability, that feels like the simplest solution

@john-science
Copy link
Member

Okay, I can jive with the high-level idea for this PR. Let's do it.

But this might be an invalid YAML file that ruamel.yaml isn't fighting us on. But even if that's so, I bet the rumael.yaml team won't change their opinion on this, and they will support it forever.

IF they do drop support for it, I won't be able to fight that. We would then just lose this feature. But I am betting the chances of that are low.

@john-science
Copy link
Member

Can we fix black for this PR? Sorry, it's a forked repo, I would do it myself.

@john-science
Copy link
Member

@drewj-usnctech Looks good! I know I technically made a bunch of comments. But they're all small.

Looks suuuper close to me.

@john-science john-science added the enhancement New feature or request label Aug 24, 2023
@john-science
Copy link
Member

@drewj-usnctech This PR is super close. Just FYI.

@drewj-usnctech
Copy link
Contributor Author

@drewj-usnctech This PR is super close. Just FYI.

Thanks @john-science. I'll try to get back to it by end of the week

…t-mod-empty-validation

* terrapower/main: (23 commits)
  Allowing users to set powerDensity instead of power (terrapower#1395)
  Fix assemNum parameter for uniform mesh reactor. (terrapower#1398)
  Update to black v22.6.0 (terrapower#1396)
  Fixing gallery example for new working Grids (terrapower#1394)
  Refactor armi.reactor.grids to wider submodule; Grids as ABC (terrapower#1373)
  Make SFP a child of reactor. (terrapower#1349)
  Move max assembly number out of global scope (terrapower#1383)
  Ensuring we throw an error on bad YAML files (terrapower#1358)
  Removing unused settings (terrapower#1393)
  Fixing some miscellaneous TODOs (terrapower#1392)
  Removing global plugin flags (terrapower#1388)
  Update reactivity coefficient parameters (terrapower#1355)
  Fixing ruff formatting of doc gallery examples (terrapower#1389)
  Fixing broken link (terrapower#1390)
  Removing unreachable code block (terrapower#1391)
  Removing unnecessary f-strings (terrapower#1387)
  Updating documentation dependencies for Python 3.11 (terrapower#1378)
  Adding a little code coverage to the CLIs (terrapower#1371)
  Remove coveragepy-lcov dependency during CI. (terrapower#1382)
  Reconnect logger after disconnecting in test. (terrapower#1381)
  ...
@john-science john-science merged commit 738f47e into terrapower:main Sep 8, 2023
10 checks passed
@drewj-usnctech drewj-usnctech deleted the drewj-usnctech/mat-mod-empty-validation branch April 10, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Material modifier validation fails if modifications are valid for some but not all blocks in an assembly
2 participants