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

Clarify definitions of crInsertedElevation and crWithdrawnElevation #1161

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

keckler
Copy link
Member

@keckler keckler commented Feb 6, 2023

Description

The definition of the crInsertedElevation parameter mistakenly referred to the length of the section with control material, when in reality it should have referred to the length of the moveable section.

This PR fixes this, and generally clarifies the definitions for these two important parameters.


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.

@@ -230,7 +230,7 @@ def _enforceNotesRestrictions(self, value):
"crWithdrawnElevation",
units="cm",
description=(
"The initial starting elevation of the moveable section of a control rod assembly when fully withdrawn. Note that this should "
"The elevation of the bottom of the bottom-most moveable section of a control rod assembly when fully withdrawn. Note that this should "
Copy link
Member

Choose a reason for hiding this comment

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

The reason this said starting is because the direction is based on interpretation of the parameter within the context of it's definition (think rods that insert from the bottom of the core). In most cases this is the interpretation though but others may implement onto this more generically. I'm okay with this but wanted to provide some background

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, yes, you're totally correct. I will change!

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, what about now?0e42c08

Copy link
Member

Choose a reason for hiding this comment

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

Nice! I'll merge

@jakehader jakehader merged commit 4fa97fa into main Feb 8, 2023
@opotowsky opotowsky deleted the keckler-patch-1 branch July 14, 2023 22:37
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.

2 participants