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

Fix bug in rotatePins() #855

Merged
merged 8 commits into from
Aug 24, 2022
Merged

Conversation

Nebbychadnezzar
Copy link
Contributor

@Nebbychadnezzar Nebbychadnezzar commented Aug 24, 2022

Description

The assignment for pinLocation in the rotatePins method is broken. I am not sure why it was set up the way it was, but it was using ParameterCollection.__setitem__() to make the assignment. The syntax used ended up setting the name argument to the tuple ("pinLocation", pinNum). That throws a TypeError from setattr(), which gets caught within __setitem__() and then instead makes an assignment to self._hist (where self is the parameter collection) because name is a tuple. That is definitely not what is intended.

This PR fixes this bug. Doc string improvements have also been attempted.


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

The assignment for pinLocation was broken. I am not sure why it was set
up the way it was, but it was using ParameterCollection.__setitem__() and
setting the `name` argument to the tuple ("pinLocation", pinNum). That
throws a TypeError from setattr(), which gets caught and then instead
makes an assigment to self._hist because `name` is a tuple. That is
definitely not what is intended.
@Nebbychadnezzar
Copy link
Contributor Author

Looping in fellow pin aficionado @mgjarrett

armi/reactor/blocks.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mgjarrett mgjarrett left a comment

Choose a reason for hiding this comment

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

Great find and fix!

@jakehader jakehader merged commit 542a010 into terrapower:main Aug 24, 2022
@jakehader jakehader deleted the updateDocString branch August 24, 2022 21:32
@john-science
Copy link
Member

Can someone help me out here?

What happens when you call rotatePins() on a Reactor that doesn't have pins? Two of three big reactors ARMI supports (off the top of my head) don't have pins.

For those users... does rotatePins() fail gracefully?

@mgjarrett
Copy link
Contributor

It's only on the HexBlock. If you have a HexBlock without pins, you probably don't have Flags.CLAD, so it will just produce a null lookup dictionary, {}.

If you do have a Flags.CLAD component with a multiplicity other than 1, it will produce a lookup table. The table will only be meaningful if your Flags.CLAD components are in a regular hexagonal lattice.

@john-science
Copy link
Member

It's only on the HexBlock.

AH! I'm just blind. (And probably sleep deprived.)

Thanks!

scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
The assignment for pinLocation was broken. I am not sure why it was set up the way it was, but it was using ParameterCollection.__setitem__() and setting the `name` argument to the tuple ("pinLocation", pinNum). That throws a TypeError from setattr(), which gets caught and then instead makes an assigment to self._hist because `name` is a tuple. That is definitely not what is intended.
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.

4 participants