From ef4fd04c8adace3b08ec994f95e36785ee7e09ec Mon Sep 17 00:00:00 2001 From: jstilley Date: Tue, 17 Oct 2023 16:19:29 -0700 Subject: [PATCH 01/13] Misc cleanup of PR text --- .github/pull_request_template.md | 8 +++----- doc/developer/tooling.rst | 8 ++++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 77e424a45..5c75cfea5 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -7,8 +7,6 @@ - - --- ## Checklist @@ -20,8 +18,8 @@ https://terrapower.github.io/armi/developer/tooling.html#good-pull-requests --> -- [ ] This PR has only one purpose or idea. -- [ ] Tests have been added/updated to verify that the new/changed code works. +- [ ] This PR has only [one purpose or idea](https://terrapower.github.io/armi/developer/tooling.html#one-idea-one-pr). +- [ ] [Tests](https://terrapower.github.io/armi/developer/tooling.html#test-it) have been added/updated to verify that the new/changed code works. @@ -31,5 +29,5 @@ - [ ] The [release notes](https://terrapower.github.io/armi/release/index.html) (location `doc/release/0.X.rst`) are up-to-date with any important changes. -- [ ] The documentation is still up-to-date in the `doc` folder. +- [ ] The [documentation](https://terrapower.github.io/armi/developer/tooling.html#document-it) is still up-to-date in the `doc` folder. - [ ] The dependencies are still up-to-date in `pyproject.toml`. diff --git a/doc/developer/tooling.rst b/doc/developer/tooling.rst index 37b366771..97ea2c8b7 100644 --- a/doc/developer/tooling.rst +++ b/doc/developer/tooling.rst @@ -46,10 +46,10 @@ Don't open until it is ready Your PR isn't complete when the code works, it is complete when the code is polished and all the tests are written and working. The idea here is: as soon as you open a PR, people will start -spending their time looking at it. And their time is valuable. An exception to this rule is that -GitHub allows you to `open a Draft PR `_ -which is a nice option if you need to open your PR early for some reason (usually testing). You -can also convert any open PR to Draft if you decide it needs more work. +spending their time looking at it. And their time is valuable. Even though GitHub allows you to +`open a Draft PR `_, this is +not the default open in ARMI. It should not be your workflow to open a Draft PR by default. We +prefer to keep the PR list as short as possible. Test It ^^^^^^^ From 5744e8aff1f04510e1a975bdbdce4e51e34ef1f1 Mon Sep 17 00:00:00 2001 From: jstilley Date: Tue, 17 Oct 2023 16:20:46 -0700 Subject: [PATCH 02/13] Removing rando materials code snippet --- armi/materials/__init__.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/armi/materials/__init__.py b/armi/materials/__init__.py index 5482ede60..319e0aeb6 100644 --- a/armi/materials/__init__.py +++ b/armi/materials/__init__.py @@ -94,16 +94,6 @@ def importMaterialsIntoModuleNamespace(path, name, namespace, updateSource=None) importMaterialsIntoModuleNamespace(__path__, __name__, globals()) -# the co_varnames attribute contains arguments and then locals so we must restrict it to just the arguments. -AVAILABLE_MODIFICATION_NAMES = { - name - for subclass in dynamicImporter.getEntireFamilyTree(Material) - for name in subclass.applyInputParams.__code__.co_varnames[ - : subclass.applyInputParams.__code__.co_argcount - ] -} -AVAILABLE_MODIFICATION_NAMES.remove("self") - def iterAllMaterialClassesInNamespace(namespace): """ From b712f2d59fb333f7c274c35b3e0b26aaad54f6ec Mon Sep 17 00:00:00 2001 From: jstilley Date: Wed, 18 Oct 2023 08:45:10 -0700 Subject: [PATCH 03/13] Removing unused safety settings.py --- armi/materials/__init__.py | 1 - armi/physics/safety/__init__.py | 4 +--- armi/physics/safety/settings.py | 18 ------------------ 3 files changed, 1 insertion(+), 22 deletions(-) delete mode 100644 armi/physics/safety/settings.py diff --git a/armi/materials/__init__.py b/armi/materials/__init__.py index 319e0aeb6..02eaa38cb 100644 --- a/armi/materials/__init__.py +++ b/armi/materials/__init__.py @@ -38,7 +38,6 @@ import inspect from armi.materials.material import Material -from armi.utils import dynamicImporter # this will frequently be updated by the CONF_MATERIAL_NAMESPACE_ORDER setting # during reactor construction (see armi.reactor.reactors.factory) diff --git a/armi/physics/safety/__init__.py b/armi/physics/safety/__init__.py index adaf6b2b4..82f82e49f 100644 --- a/armi/physics/safety/__init__.py +++ b/armi/physics/safety/__init__.py @@ -15,11 +15,9 @@ """Safety package for generic safety-related code.""" from armi import plugins -from armi.physics.safety import settings - class SafetyPlugin(plugins.ArmiPlugin): @staticmethod @plugins.HOOKIMPL def defineSettings(): - return settings.defineSettings() + return [] diff --git a/armi/physics/safety/settings.py b/armi/physics/safety/settings.py deleted file mode 100644 index 5571a74e9..000000000 --- a/armi/physics/safety/settings.py +++ /dev/null @@ -1,18 +0,0 @@ -# Copyright 2019 TerraPower, LLC -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - - -def defineSettings(): - settings = [] - return settings From 5cf77dfaaf497b5fdaad75e234028f4079548d02 Mon Sep 17 00:00:00 2001 From: jstilley Date: Wed, 18 Oct 2023 09:05:34 -0700 Subject: [PATCH 04/13] Add requirements to PR template --- .github/pull_request_template.md | 4 ++-- doc/developer/tooling.rst | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 5c75cfea5..c13378ea6 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -14,8 +14,7 @@ - [ ] This PR has only [one purpose or idea](https://terrapower.github.io/armi/developer/tooling.html#one-idea-one-pr). @@ -30,4 +29,5 @@ - [ ] The [release notes](https://terrapower.github.io/armi/release/index.html) (location `doc/release/0.X.rst`) are up-to-date with any important changes. - [ ] The [documentation](https://terrapower.github.io/armi/developer/tooling.html#document-it) is still up-to-date in the `doc` folder. +- [ ] No [requirements](https://terrapower.github.io/armi/developer/tooling.html#watch-for-requirements) were altered. - [ ] The dependencies are still up-to-date in `pyproject.toml`. diff --git a/doc/developer/tooling.rst b/doc/developer/tooling.rst index 97ea2c8b7..98e7a0205 100644 --- a/doc/developer/tooling.rst +++ b/doc/developer/tooling.rst @@ -77,6 +77,27 @@ nitty-gritty detail to fix a bug without you. Think about variable names, commen Also consider (if you are making a major change) that you might be making something in the docs out-of-date. +Watch for Requirements +^^^^^^^^^^^^^^^^^^^^^^ +When you are touching code in ARMI, watch out for the docstrings in the methods, classes, or +modules you are editing. These docstrings might have bread crumbs that link back to requirements. +Such breadcrumbs will look like: + +.. code-block:: + + """ + .. req: This is a requirement breadcrumb. + + .. test: This is a test breadcrumb. + + .. impl: This is an implementation breadcrumb. + + """ + +If you touch any code that has such a docstring, even in a file, you are going to be +responsible for not breaking that code/functionality. And you will be required to explicitly +call out that you touch such a code in your PR. + Packaging and dependency management ----------------------------------- The process of packaging Python projects and managing their dependencies is somewhat From d11af9f4ae0bb777f42900cd9aca46d9e5f0d8fa Mon Sep 17 00:00:00 2001 From: jstilley Date: Wed, 18 Oct 2023 10:40:52 -0700 Subject: [PATCH 05/13] Adding code coverage --- armi/reactor/tests/test_reactors.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/armi/reactor/tests/test_reactors.py b/armi/reactor/tests/test_reactors.py index 9cb3a4972..b144e8907 100644 --- a/armi/reactor/tests/test_reactors.py +++ b/armi/reactor/tests/test_reactors.py @@ -279,6 +279,9 @@ def test_getTotalParam(self): val2 = self.r.core.getTotalBlockParam("power", addSymmetricPositions=True) self.assertEqual(val2 / self.r.core.powerMultiplier, val) + with self.assertRaises(ValueError): + self.r.core.getTotalBlockParam(generationNum=1) + def test_geomType(self): self.assertEqual(self.r.core.geomType, geometry.GeomType.HEX) @@ -622,6 +625,11 @@ def test_getNumAssembliesWithAllRingsFilledOut(self): nAssmWithBlanks = self.r.core.getNumAssembliesWithAllRingsFilledOut(nRings) self.assertEqual(77, nAssmWithBlanks) + @unittest.mock.patch("armi.reactor.reactors.Core.powerMultiplier", 1) + def test_getNumAssembliesWithAllRingsFilledOutBipass(self): + nAssems = self.r.core.getNumAssembliesWithAllRingsFilledOut(3) + self.assertEqual(19, nAssems) + def test_getNumEnergyGroups(self): # this Core doesn't have a loaded ISOTXS library, so this test is minimally useful with self.assertRaises(AttributeError): From 1a34bc1762d8b79ff4fc425517167d066c7ac346 Mon Sep 17 00:00:00 2001 From: jstilley Date: Wed, 18 Oct 2023 10:59:10 -0700 Subject: [PATCH 06/13] Trying to fix mock import issue on GH --- armi/reactor/tests/test_reactors.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/armi/reactor/tests/test_reactors.py b/armi/reactor/tests/test_reactors.py index b144e8907..7e3fa9533 100644 --- a/armi/reactor/tests/test_reactors.py +++ b/armi/reactor/tests/test_reactors.py @@ -16,6 +16,7 @@ import logging import os import unittest +from unittest.mock import patch from numpy.testing import assert_allclose, assert_equal from six.moves import cPickle @@ -625,7 +626,7 @@ def test_getNumAssembliesWithAllRingsFilledOut(self): nAssmWithBlanks = self.r.core.getNumAssembliesWithAllRingsFilledOut(nRings) self.assertEqual(77, nAssmWithBlanks) - @unittest.mock.patch("armi.reactor.reactors.Core.powerMultiplier", 1) + @patch("armi.reactor.reactors.Core.powerMultiplier", 1) def test_getNumAssembliesWithAllRingsFilledOutBipass(self): nAssems = self.r.core.getNumAssembliesWithAllRingsFilledOut(3) self.assertEqual(19, nAssems) From fe3d21e43b9732bdf7bcddf9bb14906ebdf4df5a Mon Sep 17 00:00:00 2001 From: jstilley Date: Wed, 18 Oct 2023 13:22:15 -0700 Subject: [PATCH 07/13] Removing useless getMassFracCopy --- armi/materials/material.py | 4 ---- armi/materials/tests/test_materials.py | 4 ++-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/armi/materials/material.py b/armi/materials/material.py index e7d3d09d0..c5592db4d 100644 --- a/armi/materials/material.py +++ b/armi/materials/material.py @@ -17,7 +17,6 @@ Most temperatures may be specified in either K or C and the functions will convert for you. """ -import copy import warnings from scipy.optimize import fsolve @@ -574,9 +573,6 @@ def removeNucMassFrac(self, nuc: str) -> None: # the nuc isn't in the mass Frac vector pass - def getMassFracCopy(self): - return copy.deepcopy(self.massFrac) - def checkPropertyTempRange(self, label, val): """Checks if the given property / value combination fall between the min and max valid temperatures provided in the propertyValidTemperature object. diff --git a/armi/materials/tests/test_materials.py b/armi/materials/tests/test_materials.py index 258e0282d..e60d692be 100644 --- a/armi/materials/tests/test_materials.py +++ b/armi/materials/tests/test_materials.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. """Tests materials.py.""" - +from copy import deepcopy import math import pickle import unittest @@ -710,7 +710,7 @@ def test_duplicate(self): for key in self.mat.massFrac: self.assertEqual(duplicateU.massFrac[key], self.mat.massFrac[key]) - duplicateMassFrac = self.mat.getMassFracCopy() + duplicateMassFrac = deepcopy(self.mat.massFrac) for key in self.mat.massFrac.keys(): self.assertEqual(duplicateMassFrac[key], self.mat.massFrac[key]) From e179d884cd37c131e0e832b97cc1ba78c43b3ce8 Mon Sep 17 00:00:00 2001 From: jstilley Date: Wed, 18 Oct 2023 14:16:17 -0700 Subject: [PATCH 08/13] Adding temp dir to clean up working repo --- armi/nuclearDataIO/tests/test_xsLibraries.py | 33 ++++++++++---------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/armi/nuclearDataIO/tests/test_xsLibraries.py b/armi/nuclearDataIO/tests/test_xsLibraries.py index 692e7e93a..bbe29afe2 100644 --- a/armi/nuclearDataIO/tests/test_xsLibraries.py +++ b/armi/nuclearDataIO/tests/test_xsLibraries.py @@ -127,23 +127,24 @@ def test_mergeFailsWithNonIsotxsFiles(self): finally: os.remove(dummyFileName) - dummyFileName = "ISOtopics.txt" - with open(dummyFileName, "w") as file: - file.write( - "This is a file that starts with the letters 'ISO' but will" - " break the regular expression search." - ) - - try: - with mockRunLogs.BufferLog() as log: - lib = xsLibraries.IsotxsLibrary() - xsLibraries.mergeXSLibrariesInWorkingDirectory(lib) - self.assertIn( - f"{dummyFileName} in the merging of ISOXX files", - log.getStdout(), + with TemporaryDirectoryChanger(): + dummyFileName = "ISOtopics.txt" + with open(dummyFileName, "w") as file: + file.write( + "This is a file that starts with the letters 'ISO' but will" + " break the regular expression search." ) - finally: - pass + + try: + with mockRunLogs.BufferLog() as log: + lib = xsLibraries.IsotxsLibrary() + xsLibraries.mergeXSLibrariesInWorkingDirectory(lib) + self.assertIn( + f"{dummyFileName} in the merging of ISOXX files", + log.getStdout(), + ) + finally: + pass def _xsLibraryAttributeHelper( self, From 5cf7c774155f27e9c665bc0a15a33c948a538a61 Mon Sep 17 00:00:00 2001 From: jstilley Date: Wed, 18 Oct 2023 14:19:28 -0700 Subject: [PATCH 09/13] Removing unit test crumbs --- armi/utils/tests/test_plotting.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/armi/utils/tests/test_plotting.py b/armi/utils/tests/test_plotting.py index 802f01ae3..ec7ab50f8 100644 --- a/armi/utils/tests/test_plotting.py +++ b/armi/utils/tests/test_plotting.py @@ -55,6 +55,9 @@ def test_plotAssemblyTypes(self): plotting.plotAssemblyTypes(self.r.core.parent.blueprints, plotPath) self._checkExists(plotPath) + if os.path.exists(plotPath): + os.remove(plotPath) + plotPath = "coreAssemblyTypes2.png" plotting.plotAssemblyTypes( self.r.core.parent.blueprints, @@ -64,9 +67,15 @@ def test_plotAssemblyTypes(self): ) self._checkExists(plotPath) + if os.path.exists(plotPath): + os.remove(plotPath) + with self.assertRaises(ValueError): plotting.plotAssemblyTypes(None, plotPath, None) + if os.path.exists(plotPath): + os.remove(plotPath) + def test_plotBlockFlux(self): with TemporaryDirectoryChanger(): xslib = isotxs.readBinary(ISOAA_PATH) From 4e4a8bcf28a6c6174e760ff9fc1a63b9ae8d16fa Mon Sep 17 00:00:00 2001 From: jstilley Date: Thu, 19 Oct 2023 09:06:03 -0700 Subject: [PATCH 10/13] Removing unused variable cycleTime --- armi/physics/fuelCycle/fuelHandlerInterface.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/armi/physics/fuelCycle/fuelHandlerInterface.py b/armi/physics/fuelCycle/fuelHandlerInterface.py index 31732123f..aa7098bbd 100644 --- a/armi/physics/fuelCycle/fuelHandlerInterface.py +++ b/armi/physics/fuelCycle/fuelHandlerInterface.py @@ -42,8 +42,6 @@ def __init__(self, r, cs): # need order due to nature of moves but with fast membership tests self.moved = [] self.cycle = 0 - # filled during summary of EOC time in years of each cycle (time at which shuffling occurs) - self.cycleTime = {} @staticmethod def specifyInputs(cs): @@ -86,9 +84,6 @@ def interactBOC(self, cycle=None): self.manageFuel(cycle) def interactEOC(self, cycle=None): - timeYears = self.r.p.time - # keep track of the EOC time in years. - self.cycleTime[cycle] = timeYears if self.r.sfp is not None: runLog.extra( f"There are {len(self.r.sfp)} assemblies in the Spent Fuel Pool" From 782f9f5c3f3a21194b013208307082c937fdb4c6 Mon Sep 17 00:00:00 2001 From: jstilley Date: Thu, 19 Oct 2023 10:43:17 -0700 Subject: [PATCH 11/13] Fixing bad comment --- armi/bookkeeping/visualization/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/armi/bookkeeping/visualization/utils.py b/armi/bookkeeping/visualization/utils.py index 8bef3bd91..a27a0a3e0 100644 --- a/armi/bookkeeping/visualization/utils.py +++ b/armi/bookkeeping/visualization/utils.py @@ -255,7 +255,7 @@ def _createCartesianBlockMesh(b: blocks.CartesianBlock) -> VtkMesh: def _createTRZBlockMesh(b: blocks.ThRZBlock) -> VtkMesh: - # There's no sugar-coating this one. It sucks. + # This could be improved. rIn = b.radialInner() rOut = b.radialOuter() thIn = b.thetaInner() From 85575c84178504f7c73117f8d2ecbb97713b95ea Mon Sep 17 00:00:00 2001 From: jstilley Date: Thu, 19 Oct 2023 10:59:48 -0700 Subject: [PATCH 12/13] Tweaking test to be more stable in GitHub's working dirs --- armi/bookkeeping/report/tests/test_report.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/armi/bookkeeping/report/tests/test_report.py b/armi/bookkeeping/report/tests/test_report.py index d1dc80c68..577b97276 100644 --- a/armi/bookkeeping/report/tests/test_report.py +++ b/armi/bookkeeping/report/tests/test_report.py @@ -136,11 +136,8 @@ def test_reactorSpecificReporting(self): def test_writeWelcomeHeaders(self): o, r = loadTestReactor() - # grab a random file (that exist in the working dir) - files = os.listdir(os.getcwd()) - files = [f for f in files if f.endswith(".py") or f.endswith(".txt")] - self.assertGreater(len(files), 0) - randoFile = files[0] + # grab this file path + randoFile = os.path.abspath(__file__) # pass that random file into the settings o.cs["crossSectionControl"]["DA"].xsFileLocation = randoFile From 35ce8c2678af8347003559f558d2d119ca55b61c Mon Sep 17 00:00:00 2001 From: jstilley Date: Thu, 19 Oct 2023 11:45:49 -0700 Subject: [PATCH 13/13] Moving this blurb to another PR --- doc/developer/tooling.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/developer/tooling.rst b/doc/developer/tooling.rst index 98e7a0205..d9dfb091c 100644 --- a/doc/developer/tooling.rst +++ b/doc/developer/tooling.rst @@ -46,10 +46,10 @@ Don't open until it is ready Your PR isn't complete when the code works, it is complete when the code is polished and all the tests are written and working. The idea here is: as soon as you open a PR, people will start -spending their time looking at it. And their time is valuable. Even though GitHub allows you to -`open a Draft PR `_, this is -not the default open in ARMI. It should not be your workflow to open a Draft PR by default. We -prefer to keep the PR list as short as possible. +spending their time looking at it. And their time is valuable. An exception to this rule is that +GitHub allows you to `open a Draft PR `_ +which is a nice option if you need to open your PR early for some reason (usually testing). You +can also convert any open PR to Draft if you decide it needs more work. Test It ^^^^^^^