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

Remove unused HCFcoretype setting #1179

Merged
merged 2 commits into from
Feb 13, 2023

Conversation

mgjarrett
Copy link
Contributor

@mgjarrett mgjarrett commented Feb 13, 2023

Description

The HCFcoretype setting is not used anywhere in the framework or in downstream TerraPower projects.

Since the only options are TWR*, I assume nobody outside of TerraPower is using this setting either.

TerraPower TH solvers use an assembly parameter to track hot channel factors:

pb.defParam(
"hotChannelFactors",
units="None",
description="Definition of set of HCFs to be applied to assembly.",
location="?",
default="Default",
saveToDB=True,
categories=[parameters.Category.assignInBlueprints],
)

I assume anybody else wanting to apply hot channel factors would want to use the same assembly parameter, and not this case setting.


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.

@mgjarrett mgjarrett marked this pull request as ready for review February 13, 2023 21:07
@mgjarrett
Copy link
Contributor Author

The coverage reduction is from two uncovered lines here:

armi/armi/runLog.py

Lines 649 to 651 in 0576560

except FileExistsError:
# If we hit this race condition, we still win.
return

I don't know how we were hitting the FileExistsError before, or how we could force it to be hit again. It's not clear how it relates to this PR.

Copy link
Member

@opotowsky opotowsky left a comment

Choose a reason for hiding this comment

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

Nice cleanup catch! Somehow I missed it when investigating the HCF settings...

That is a weird reason for a coverage drop. Sometimes it happens if the base branch isn't perfectly in sync with main, and even though github is smart enough to handle that situation, coveralls might not be.

@opotowsky opotowsky added the cleanup Code/comment cleanup: Low Priority label Feb 13, 2023
@opotowsky opotowsky merged commit 1a4cb68 into terrapower:main Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants