Repository: terrapower/armi
Pull request #2046 (2025-01-06 16:30:49)

Making material modifications more robust against type errors
## What is the change?

This PR adds a deeper interrogation of the error conditions that come up during the application of material modifications. The intention is to catch when the user may have specified a material modification of the wrong type in their blueprints, which before would have been silently ignored.

## Why is the change being made?

I came across an instance in my own work where a material modification of the wrong type was being specified. This was a bug that had been in my model for multiple years, and I was surprised to find that ARMI did not seem to care to warn me.

The root of this problem is the way that the material modifications infrastructure is designed. Material modifications are applied during the construction of components here:
https://github.com/terrapower/armi/blob/1920ff0ecb146a050c1c2f0a199cf620834728b2/armi/reactor/blueprints/componentBlueprint.py#L367-L375

As can be seen above, the whole thing is wrapped in a `try/except`, with the only error condition being checked a `ValueError`. The assumption with that code is that the `ValueError` will only be triggered when a material modification with an invalid name is passed to the material's `applyInputParams()` method. That is a very bold assumption, as basically anything could go wrong during the execution of `applyInputParams()`, _including`ValueError`s that are indicative of real bugs_. 

As a concrete example, let's look at the material modifications within `UraniumOxide`:
https://github.com/terrapower/armi/blob/main/armi/materials/uraniumOxide.py#L109-L132

That material will take `U235_wt_frac` and `TD_frac`, both of which _should_ be `float`s. But from my yaml blueprints file it would be entirely possible for me to specify a `str`. This would obviously break `UraniumOxide.applyInputParams()`, which would raise a `ValueError`. However when that `ValueError` is raised, the `try/except` around `applyInputParams()` will just `pass` and the material modification is left at its default value. This would be a bug.

---

## Checklist

<!--
    You (the pull requester) should put an `x` in the boxes below you have completed.

    (If a checkbox doesn't apply to your PR, check it anyway.)
-->

- [x] This PR has only [one purpose or idea](https://terrapower.github.io/armi/developer/tooling.html#one-idea-one-pr).
- [x] [Tests](https://terrapower.github.io/armi/developer/tooling.html#test-it) have been added/updated to verify any new/changed code.

<!-- Check the code quality -->

- [x] The code style follows [good practices](https://terrapower.github.io/armi/developer/standards_and_practices.html).
- [x] The commit message(s) follow [good practices](https://terrapower.github.io/armi/developer/tooling.html).

<!-- Check the project-level cruft -->

- [x] The [release notes](https://terrapower.github.io/armi/developer/tooling.html#add-release-notes) have been updated if necessary.
- [x] The [documentation](https://terrapower.github.io/armi/developer/tooling.html#document-it) is still up-to-date in the `doc` folder.
- [x] The dependencies are still up-to-date in `pyproject.toml`.

john-science: I like the change. But what I would REALLY like is a two test cases, one that hit each version of this exception. Just to be clear about what we expect each type of input to do.

Is that too much work?
keckler: > But what I would REALLY like is a two test cases, one that hit each version of this exception. Just to be clear about what we expect each type of input to do.

Yes, definitely. I actually thought I wrote some tests here, but I don't seem them in my local copy... I guess I was probably just doing one-off testing.

Yes, I will write some tests.
keckler: > I like the change. But what I would REALLY like is a two test cases, one that hit each version of this exception. Just to be clear about what we expect each type of input to do.

@john-science I added a unit test to the PR that catches the new scenario where the specified material modification is of the wrong type. Specifically, it catches this part:
https://github.com/terrapower/armi/pull/2046/files#diff-adf715a7067ab05bf0ada9c5401f6b7a9b75ffd67dddb7b767550af593175ad6R381-R385

The [other part](https://github.com/terrapower/armi/pull/2046/files#diff-adf715a7067ab05bf0ada9c5401f6b7a9b75ffd67dddb7b767550af593175ad6R375-R379) (where the error message would have resulted from a material modification with the wrong name being applied to a component) is already going to be caught by an existing test:
https://github.com/terrapower/armi/blob/ee691abf61d56197515886ec5bbb4804b4673a90/armi/reactor/blueprints/tests/test_materialModifications.py#L302-L348

So I think that this PR is good to go, but me know what you think.
john-science: 1. Could use a release note under "Bug Fixes".
2. Have you run this against downstream test suites?
keckler: > Have you run this against downstream test suites?

Yes, I discovered this issue from one of my existing models, and have since tested this PR against that use case. 
john-science: Okay, this passed my downstream testing. Merging.

Patch:
diff --git a/armi/reactor/blueprints/componentBlueprint.py b/armi/reactor/blueprints/componentBlueprint.py
index 54676cbe..838c5525 100644
--- a/armi/reactor/blueprints/componentBlueprint.py
+++ b/armi/reactor/blueprints/componentBlueprint.py
@@ -378,10 +378,18 @@ class ComponentBlueprint(yamlize.Object):
             try:
                 # update material with updated input params from blueprints file.
                 mat.applyInputParams(**matMods)
-            except TypeError:
-                # This component does not accept material modification inputs of the names passed in
-                # Keep going since the modification could work for another component
-                pass
+            except TypeError as ee:
+                errorMessage = ee.args[0]
+                if "got an unexpected keyword argument" in errorMessage:
+                    # This component does not accept material modification inputs of the names passed in
+                    # Keep going since the modification could work for another component
+                    pass
+                else:
+                    raise ValueError(
+                        f"Something went wrong in applying the material modifications {matMods} "
+                        f"to component {self.name}.\n"
+                        f"Error message is: \n{errorMessage}."
+                    )
 
         expandElementals(mat, blueprint)
 


Test patch:
diff --git a/armi/reactor/blueprints/tests/test_materialModifications.py b/armi/reactor/blueprints/tests/test_materialModifications.py
index 52ada251..eaccb00d 100644
--- a/armi/reactor/blueprints/tests/test_materialModifications.py
+++ b/armi/reactor/blueprints/tests/test_materialModifications.py
@@ -259,7 +259,7 @@ assemblies:
 
     def test_invalidMatModName(self):
         """
-        This test shows proves that we can detect invalid material modification
+        This test shows that we can detect invalid material modification
         names when they are specified on an assembly blueprint. We happen to know
         that ZR_wt_frac is a valid modification for the UZr material class, so we
         use that in the first call to prove that things initially work fine.
@@ -299,6 +299,35 @@ assemblies:
         """
             )
 
+    def test_invalidMatModType(self):
+        """
+        This test shows that we can detect material modifications that are invalid
+        because of their values, not just their names.
+        We happen to know that ZR_wt_frac is a valid modification for UZr, so we
+        use that in the first call to prove that things initially work fine.
+        """
+        a = self.loadUZrAssembly(
+            """
+        material modifications:
+            ZR_wt_frac: [1]
+        """
+        )
+        # just to prove that the above works fine before we modify it
+        self.assertAlmostEqual(a[0][0].getMassFrac("ZR"), 1)
+
+        with self.assertRaises(ValueError) as ee:
+            a = self.loadUZrAssembly(
+                """
+        material modifications:
+            ZR_wt_frac: [this_is_a_value_of_incompatible_type]
+        """
+            )
+
+            self.assertIn(
+                "Something went wrong in applying the material modifications",
+                ee.args[0],
+            )
+
     def test_matModsUpTheMRO(self):
         """
         Make sure that valid/invalid material modifications are searched up