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

Harmonize material names and class names #1270

Merged
merged 4 commits into from
May 19, 2023

Conversation

alexhjames
Copy link
Contributor

@alexhjames alexhjames commented May 18, 2023

Description

For materials.resolveMaterialClassByName() to function, material.name and the class name should be the same. This PR ensures that is true for all materials.

No testing was added as the name field on the material isn't really something to be tested.


Checklist

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

@keckler
Copy link
Member

keckler commented May 18, 2023

If we're gonna do this, I welcome you to fix all the broken blueprints files in our benchmark models :-)

Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

Approved, pending testing.

@alexhjames
Copy link
Contributor Author

If we're gonna do this, I welcome you to fix all the broken blueprints files in our benchmark models :-)

I don't think this needed. We ran into this in terrapower-twr a few months ago with T92Depleted vs T92-Depleted-Boron. The blueprints use the class name not the name field.

@keckler
Copy link
Member

keckler commented May 18, 2023

Well I'll be darned, you're right. Forget my snarky comment!

@john-science
Copy link
Member

john-science commented May 18, 2023

Two unit tests failing:

Traceback (most recent call last):
  File "/home/runner/work/armi/armi/armi/tests/test_lwrInputs.py", line 67, in test_loadC5G7
    self.assertIn("Uranium Oxide", streamVal, msg=streamVal)
  File "/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/unittest/case.py", line 1179, in assertIn
    self.fail(self._formatMessage(msg, standardMsg))
  File "/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/unittest/case.py", line 753, in fail
    raise self.failureException(msg)
AssertionError: 'Uranium Oxide' not found in '[warn] Ignoring invalid settings: fakeBad\n=========== Settings Validation Checks ===========\n[warn] Burn chain already imposed. Skipping 

and

[gw1] linux -- Python 3.8.16 /home/runner/work/armi/armi/.tox/test/bin/python
Traceback (most recent call last):
  File "/home/runner/work/armi/armi/armi/reactor/blueprints/tests/test_reactorBlueprints.py", line 119, in test_materialDataSummary
    self.assertEqual(actual, expected)
  File "/opt/hostedtoolcache/Python/3.8.16/x[64](https://github.com/terrapower/armi/actions/runs/5018906168/jobs/8998833243?pr=1270#step:7:65)/lib/python3.8/unittest/case.py", line 912, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/unittest/case.py", line 1129, in assertTupleEqual
    self.assertSequenceEqual(tuple1, tuple2, msg, seq_type=tuple)
  File "/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/unittest/case.py", line 1100, in assertSequenceEqual
    self.fail(msg)
  File "/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/unittest/case.py", line 753, in fail
    raise self.failureException(msg)
AssertionError: Tuples differ: ('Custom', 'ARMI', False) != ('Custom Material', 'ARMI', False)

@alexhjames
Copy link
Contributor Author

@john-science Two unit tests are updated! Sorry I didn't catch those locally

@john-science john-science merged commit 7603143 into terrapower:main May 19, 2023
9 checks passed
@sombrereau
Copy link
Contributor

@alexhjames / @john-science This broke a few of my internal unittests because of name changes to Uranium Oxide and Zirconium. No big deal, but I think we need to do a better job communicating these kinds of changes ... or maybe on my end listening for these kinds of changes.

drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Jun 1, 2023
* main: (23 commits)
  block converters preserve flags (terrapower#1271)
  Fixed float decimals v3 (terrapower#1283)
  Fixed string format error for snapshots (terrapower#1277)
  Pinning the versions of yamlize and ruamel (terrapower#1281)
  Releasing v0.2.7 (terrapower#1276)
  Creating verisons setting section (terrapower#1274)
  Adding to GlobalFluxOptions docstring (terrapower#1273)
  Harmonize material names and class names (terrapower#1270)
  Removing Component.getMassDensity (terrapower#1266)
  Cleaning up language for our versioning rules (terrapower#1175)
  Fixing the Settings Report in the Docs (terrapower#1264)
  Fixing getTemperatureAtDensity to use non-pseduo density (terrapower#1262)
  Fixing settings report docs from terrapower#1207 (terrapower#1263)
  Update test_components.py (terrapower#1261)
  Update gamma Uniform Mesh Converter (terrapower#1213)
  Updating way that settings report defaults are printed (terrapower#1207)
  Fixing UserPlugins.defineFlag (terrapower#1241)
  quick bug fix for PR 1239 (terrapower#1260)
  New Option to Control Lattice Physics Update Frequency (terrapower#1239)
  Fix a bug in database compare. (terrapower#1258)
  ...
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.

None yet

4 participants