Skip to content

Commit

Permalink
Misc cleanup (#1439)
Browse files Browse the repository at this point in the history
- I removed some random code (AVAILABLE_MODIFICATION_NAMES)
- I removed an unused armi/physics/safety/settings.py file.
- I added requirements to our PR template.
- Removing useless getMassFracCopy
- Removing unused variable cycleTime
  • Loading branch information
john-science committed Oct 19, 2023
1 parent bdd34d3 commit b67c22f
Show file tree
Hide file tree
Showing 13 changed files with 67 additions and 72 deletions.
12 changes: 5 additions & 7 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,18 @@
<!-- MANDATORY: Explain why the change is necessary -->
<!-- Optional: Link to any related GitHub Issues -->



---

## Checklist

<!--
You (the pull requester) should put an `x` in the boxes below you have completed.
If you're unsure about any of them, don't hesitate to ask. We're here to help!
Learn what a "good PR" looks like here:
https://terrapower.github.io/armi/developer/tooling.html#good-pull-requests
(If a checkbox requires no action for this PR, put an `x` in the box.)
-->

- [ ] 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.

<!-- Check the code quality -->

Expand All @@ -31,5 +28,6 @@
<!-- Check the project-level cruft -->

- [ ] 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.
- [ ] 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`.
7 changes: 2 additions & 5 deletions armi/bookkeeping/report/tests/test_report.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion armi/bookkeeping/visualization/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
11 changes: 0 additions & 11 deletions armi/materials/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -94,16 +93,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):
"""
Expand Down
4 changes: 0 additions & 4 deletions armi/materials/material.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions armi/materials/tests/test_materials.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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])

Expand Down
33 changes: 17 additions & 16 deletions armi/nuclearDataIO/tests/test_xsLibraries.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 0 additions & 5 deletions armi/physics/fuelCycle/fuelHandlerInterface.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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"
Expand Down
4 changes: 1 addition & 3 deletions armi/physics/safety/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 []
18 changes: 0 additions & 18 deletions armi/physics/safety/settings.py

This file was deleted.

9 changes: 9 additions & 0 deletions armi/reactor/tests/test_reactors.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -279,6 +280,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)

Expand Down Expand Up @@ -622,6 +626,11 @@ def test_getNumAssembliesWithAllRingsFilledOut(self):
nAssmWithBlanks = self.r.core.getNumAssembliesWithAllRingsFilledOut(nRings)
self.assertEqual(77, nAssmWithBlanks)

@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):
Expand Down
9 changes: 9 additions & 0 deletions armi/utils/tests/test_plotting.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
Expand Down
21 changes: 21 additions & 0 deletions doc/developer/tooling.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit b67c22f

Please sign in to comment.