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

Use b.core instead of b.r.core. #909

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

mgjarrett
Copy link
Contributor

@mgjarrett mgjarrett commented Sep 29, 2022

Description

Blocks have an r property and a core property. Both functions just look up the chain of ancestors of the block to find the core and reactor. It's unnecessary to go all the way up to the reactor just to then grab the core off of it.


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.

Blocks have an r property and a core property. Both functions just look
up the chain of ancestors of the block to find the core and reactor.
It's unnecessary to go all the way up to the reactor just to then grab
the core off of it.
@mgjarrett
Copy link
Contributor Author

Basically acting on the note in this docstring:

armi/armi/reactor/blocks.py

Lines 173 to 181 in 646f23d

It may make sense to try to factor out usage of ``b.r``.
For now, this is presumptive of the structure of the composite hierarchy; i.e.
the parent of a CORE must be the reactor. Fortunately, we probably don't
ultimately want to return the reactor in the first place. Rather, we probably want the core
anyways, since practically all `b.r` calls are historically `b.r.core`. It may be
prefereable to remove this property, replace with `self.core`, which can return the core.
Then refactor all of the b.r.cores, to b.core.
"""

Can't completely remove b.r yet because it's used to get active nuclides from reactor blueprints here:

if any(nuc in self.r.blueprints.activeNuclides for nuc in adjustList):

@mgjarrett mgjarrett changed the title Use self.core instead of self.r.core. Use b.core instead of b.r.core. Sep 29, 2022
@john-science john-science added the architecture Issues related to big picture system architecture label Sep 29, 2022
@john-science
Copy link
Member

I love it!

Can we remove the comment you link on line ~173 now?

@mgjarrett
Copy link
Contributor Author

Can we remove the comment you link on line ~173 now?

I would like to remove that entire function, but we still have one case of b.r.blueprints so we can't remove this property altogether. I think we can trim some of the docstring and make it better. I'll push a commit with a new docstring.

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.

I think these are good changes.

With regards to your comment:

Can't completely remove b.r yet because it's used to get active nuclides from reactor blueprints here:

Is there a reason why we want to remove b.r completely? I agree that, in the cases you changed b.r.core -> b.core, it makes sense because it avoids unnecessary computation. But I think that b.r could still be useful in the API.

Plus, if we're trying to avoid changes to the API, I would view that as an unnecessary change to the API.

Just a thought.

@mgjarrett
Copy link
Contributor Author

I agree that removing b.r would be an unnecessary change to the API. The docstring had this comment:

It may make sense to try to factor out the usage of ``b.r``

So that's why I thought it might be desirable to remove it. But as long as it's being used, and it's not hurting anything, it makes more sense to leave it in for continuity.

@john-science john-science merged commit 6a01e97 into terrapower:main Sep 30, 2022
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Issues related to big picture system architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants