Using CONF_XXX variables

[Till223]

Description

This is a part of the solution for issue #1103

I went through three of the files that are listed in the issue, identified the CONF variables, searched the project for their string literal, replaced any occurrence where it seemed appropriate and added the required imports.

These are the mentioned three files:

• armi/physics/fuelPerformance/settings.py
• armi/physics/neutronics/fissionProductModel/fissionProductModelSettings.py
• armi/physics/neutronics/settings.py

The tests currently don't run anymore because of circular imports through neutronics/settings.py . This isn't particularly surprising as that file has a lot of CONF variables, which means it got imported a bunch of times . I'm not sure how to deal with this issue, my only idea would be to move the CONF variables to a separate file. What do you think?

---

Checklist

• This PR has only one purpose or idea.
• Tests have been added/updated to verify that the new/changed code works.

• The code style follows good practices.
• The commit message(s) follow good practices.

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

[CLAassistant]

All committers have signed the CLA.

[john-science]

The tests currently don't run anymore because of circular imports through neutronics/settings.py.

This is solvable. First off all, we could import the CONF_ outside the usual top-level. That will fix most of the circular imports.

OR it might just be that we have to refactor a little code. I'll wait for the tests to finish to see what the situtation is.

Thanks!

[Till223]

Sounds good, although I'm not sure I understand what you mean in this:

This is solvable. First off all, we could import the CONF_ outside the usual top-level. That will fix most of the circular imports.

Like move the 'import CONF_XXX' statement down in the file to the place they are first needed?

[jakehader]

Sounds good, although I'm not sure I understand what you mean in this:

This is solvable. First off all, we could import the CONF_ outside the usual top-level. That will fix most of the circular imports.

Like move the 'import CONF_XXX' statement down in the file to the place they are first needed?

I think this means try to add the import statements into the function they are used rather than importing on the module level. I think once this is moved it probably should work as John stated. If so, it would be good to add a comment above the import statements that is a future developer reminder.

Thanks for the work on this!

[john-science]

@Till223 For instance, at the top of globalFluxInterface.py, you have this:

from armi.physics.neutronics.settings import (
    CONF_BOUNDARIES,
    CONF_DPA_PER_FLUENCE,
    CONF_EIGEN_PROB,
    CONF_NEUTRONICS_KERNEL,
    CONF_RESTART_NEUTRONICS,
    CONF_ACLP_DOSE_LIMIT,
    CONF_DPA_XS_SET,
    CONF_GRID_PLATE_DPA_XS_SET,
    CONF_LOAD_PAD_ELEVATION,
    CONF_LOAD_PAD_LENGTH,
    CONF_XS_KERNEL,
)

But most (or all?) of these are only using in the one method: def fromUserSettings(self, cs):. So, we could move these imports down INSIDE this method. And then mark that method with # pylint: disable=import-outside-toplevel, as you can see done in a bunch of other places in ARMI where we are worried about circular imports.

It should be easy to do. The "trick" will be to find the minimum number of places you can do this to get the unit tests running again.

For that, I would look at the console logs, which will probably point you in the right direction. For instance:

ImportError: cannot import name 'GAMMA' from 'armi.physics.neutronics'

Give it a whirl, and tell me what you think.

[Till223]

Alright I've moved some of the imports, the tests pass locally for me now. I went one error at a time, so this should be the minimum number of required changes. Thanks for all the assistance :)

[Till223]

Ok, this should be ready for merging now, I'm just not too sure about the commit history. There are some unnecessary commits in there, like the "test code action", and I could probably remove or squash those with a rebase, but when I tried doing that on a separate branch just now it somehow duplicated all of my commits on the next git pull. Do you think this kind of clean up is necessary or is it fine as it is?

[john-science]

Ok, this should be ready for merging now, I'm just not too sure about the commit history...

Don't worry about the commit history. In (nearly) every ARMI PR we use the feature "Squash and Merge", so that all the commits in the PR are squashed into one PR.

And we might edit the commit message, to be useful, and all.

But this makes the commit history MUCH easier to read and understand, when we have 1 commit = 1 PR = 1 idea all through our project's commit history.

So, no worries.

Although! If you wanted to clean up the commit history in your PR and change it to be one or more PRs, you would use git rebase. It's a super powerful tool (in other situations). You could check it out!

[john-science]

@opotowsky I think we are going to wait until after the big release (on Friday?) for this PR. Much like the density3() PR. I think they are both low risk. But they touch so many files, I don't want to clog up the works on your big release.

Just #FYI.

[opotowsky]

Noted and agreed -- thanks!

[john-science]

Looks great!

Thanks for all the hard work!

I'm sorry this took so long to merge. Lame. We had a ton of activity going on and people asked me to wait on PRs not related to their big project. #shrug