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.

Sign up for GitHub

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

Jump to bottom

Move max assembly number out of global scope #1383

Merged
merged 15 commits into from
Aug 22, 2023
Merged

Move max assembly number out of global scope #1383

merged 15 commits into from
Aug 22, 2023

Conversation

john-science
Copy link
Member

What is the change?

This PR moves the "maximum assembly number" out of the global scope and into a Reactor Parameter. (The reason it's a Parameter and not just a class attribute is so we can store it in the Database.)

Why is the change being made?

We have far too many global variables in ARMI. Such a mess. This particular change was spurred by a discussion @opotowsky stared.

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 important changes.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@john-science john-science added the enhancement New feature or request label Aug 9, 2023
@john-science john-science changed the title Mv assem num 2 reactor Move max assembly number out of global scope Aug 9, 2023
ntouran
ntouran reviewed Aug 10, 2023
View reviewed changes
armi/bookkeeping/db/database3.py Show resolved Hide resolved
armi/reactor/reactors.py Show resolved Hide resolved
armi/bookkeeping/db/tests/test_database3.py Show resolved Hide resolved
@john-science john-science requested review from albeanth and keckler and removed request for mgjarrett and keckler August 17, 2023 20:25
@albeanth
Copy link
Member

@john-science Though a big fan, has this been run through the slew of internal downstream tests? I imagine it might break some things. Not sure though.

keckler
keckler reviewed Aug 17, 2023
View reviewed changes
Copy link
Member

@keckler keckler left a comment

Choose a reason for hiding this comment

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

This was more complicated than I expected. Some comments for you.

armi/reactor/converters/geometryConverters.py Show resolved Hide resolved
armi/reactor/reactors.py Show resolved Hide resolved
armi/reactor/reactors.py Show resolved Hide resolved
armi/reactor/reactors.py Outdated Show resolved Hide resolved
armi/reactor/assemblies.py Show resolved Hide resolved
armi/reactor/tests/test_reactors.py Show resolved Hide resolved
@jakehader
Copy link
Member

Maybe way to late to the party, but why do we need a counter on the assembly numbers? Is this to give it a unique ID that has never existed before? Do we have a set of requirements for naming or numbering things that we can point to or develop as a part of this PR?

I am just thinking that maxAssemNumber is basically just the len(self). Is there a way to trigger parameter updates automatically like an event trigger?

@john-science
Copy link
Member Author

@jakehader

But why do we need a counter on the assembly numbers? Is this to give it a unique ID that has never existed before?

I am not adding or removing any features from ARMI.

The problem is that all assemblies and blocks in the reactor are numbered. But currently that number is kept in a global variable that is set when you do import armi.

And this is causing all kinds of problems:

  1. It makes writing tests harder.
  2. In particular, these global variables are causing testing trouble in some of our internal repos.
  3. It means you can't have more than two reactors in Python at the same time.
    a . Or, you can, but you get impossible-to-solve invisible bugs that pop up during simulations.
  4. It makes the code a LOT harder to debug.

So, chalk this up to "code quality and maintainability". None of this is a surprising: keeping important data in global variables is a classic bad code problem.

@john-science john-science merged commit da28372 into main Aug 22, 2023
21 checks passed
@john-science john-science deleted the mv_assem_num_2_reactor branch August 22, 2023 21:47
drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Aug 24, 2023
@drewj-usnctech
Merge remote-tracking branch 'terrapower/main' into drewj-usnctech/gr…
df4bfa4 
…id-refactor-pre-1278

* terrapower/main:
  Move max assembly number out of global scope (terrapower#1383)
  Ensuring we throw an error on bad YAML files (terrapower#1358)
  Removing unused settings (terrapower#1393)
  Fixing some miscellaneous TODOs (terrapower#1392)
  Removing global plugin flags (terrapower#1388)
  Update reactivity coefficient parameters (terrapower#1355)
  Fixing ruff formatting of doc gallery examples (terrapower#1389)
  Fixing broken link (terrapower#1390)
  Removing unreachable code block (terrapower#1391)
  Removing unnecessary f-strings (terrapower#1387)
  Updating documentation dependencies for Python 3.11 (terrapower#1378)
  Adding a little code coverage to the CLIs (terrapower#1371)
  Remove coveragepy-lcov dependency during CI. (terrapower#1382)
  Reconnect logger after disconnecting in test. (terrapower#1381)
  Add Python 3.11 to GH Actions (terrapower#1341)
  Removing global variable from runLog.py (terrapower#1370)
  Moving the First-Time Contributors guide to the docs (terrapower#1368)
@opotowsky
Copy link
Member

@keckler @john-science @albeanth

This PR was amazing! But it broke both unit and integration tests internally. I just wanted to give everyone some feedback on that so we can be better about knowing when to request that certain updates are tested internally. I definitely am also to blame -- I should have realized the potential for this PR to stir the pot.

drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Sep 6, 2023
@drewj-usnctech
Merge remote-tracking branch 'terrapower/main' into drewj-usnctech/ma…
6c1ab49 
…t-mod-empty-validation

* terrapower/main: (23 commits)
  Allowing users to set powerDensity instead of power (terrapower#1395)
  Fix assemNum parameter for uniform mesh reactor. (terrapower#1398)
  Update to black v22.6.0 (terrapower#1396)
  Fixing gallery example for new working Grids (terrapower#1394)
  Refactor armi.reactor.grids to wider submodule; Grids as ABC (terrapower#1373)
  Make SFP a child of reactor. (terrapower#1349)
  Move max assembly number out of global scope (terrapower#1383)
  Ensuring we throw an error on bad YAML files (terrapower#1358)
  Removing unused settings (terrapower#1393)
  Fixing some miscellaneous TODOs (terrapower#1392)
  Removing global plugin flags (terrapower#1388)
  Update reactivity coefficient parameters (terrapower#1355)
  Fixing ruff formatting of doc gallery examples (terrapower#1389)
  Fixing broken link (terrapower#1390)
  Removing unreachable code block (terrapower#1391)
  Removing unnecessary f-strings (terrapower#1387)
  Updating documentation dependencies for Python 3.11 (terrapower#1378)
  Adding a little code coverage to the CLIs (terrapower#1371)
  Remove coveragepy-lcov dependency during CI. (terrapower#1382)
  Reconnect logger after disconnecting in test. (terrapower#1381)
  ...
@john-science john-science mentioned this pull request Oct 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonbmeng jasonbmeng jasonbmeng left review comments

@keckler keckler keckler approved these changes

@ntouran ntouran Awaiting requested review from ntouran

@albeanth albeanth Awaiting requested review from albeanth

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@john-science @albeanth @jakehader @opotowsky @keckler @ntouran @jasonbmeng