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

Misc cleanup #1439

Merged
merged 15 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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