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

(API Breaking) Finish removal of block.r #1425

Merged
merged 20 commits into from
Apr 25, 2024

Conversation

albeanth
Copy link
Member

@albeanth albeanth commented Oct 5, 2023

What is the change?

This follows up on #909 and fully removes the block.r property. Leaving it causes some (imo) unnecessary tangling. So instead of the current

Reactor <--> Core <--> Assembly <--> Block <--> Component
   ^           ^                       |
   |-----------|-----------------------|

the following would be the new Composite relationships via properties

Reactor <--> Core <--> Assembly <--> Block <--> Component
               ^                       |
               |-----------------------|

Why is the change being made?

I can whip up some examples, but it causes some tangles with testing. Plug in the docstring it even says we should probably get rid of it. Plus you can still access the reactor object via b.core.


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

@albeanth
Copy link
Member Author

albeanth commented Oct 6, 2023

Since this is an API change, it'll need to be run through the gamut of downstream tests.

@albeanth albeanth marked this pull request as draft October 6, 2023 15:57
@albeanth
Copy link
Member Author

albeanth commented Oct 6, 2023

Converting to draft. This is looking like a fairly intrusive PR.

@albeanth albeanth added the architecture Issues related to big picture system architecture label Oct 6, 2023
- in ARMI and at least one major downstream project, these are only every used on blocks. So these can be safely moved off of composites.py and onto blocks.py.
- also, given the current properties, only Blocks are even able to use these methods. assembly.r and component.r don't exist so they wouldn't work anyway. and Core.getMicroSuffix doesn't exist so Core wouldn't work either. These methods really only work for blocks, so let's move them to blocks.py.
@albeanth albeanth changed the title Finish removal of block.r (API Breaking) Finish removal of block.r Oct 6, 2023
@albeanth
Copy link
Member Author

albeanth commented Oct 6, 2023

The coverage report doesn't seem right again. I'm seeing the same coverage report results over several PRs. Something is fishy with coveralls. Any ideas?

@albeanth albeanth marked this pull request as ready for review October 11, 2023 15:07
@john-science
Copy link
Member

The coverage report doesn't seem right again. I'm seeing the same coverage report results over several PRs. Something is fishy with coveralls. Any ideas?

Fixed.

@@ -1277,124 +1276,6 @@ def getNumberDensities(self, expandFissionProducts=False):
return self._expandLFPs(numberDensities)
return numberDensities

def getNeutronEnergyDepositionConstants(self):
Copy link
Member

Choose a reason for hiding this comment

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

Why are we moving this over to the other file?

How is this related to the block.r 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.

See the commit message for #1425 (comment).

TL/DR: Those methods called self.r.core which blocked the PR from going through. After grepping ARMI and one major downstream project, those methods never get called on any Composite other than Block. So those methods can safely be moved over to the Block class and self.r.core can get replaced with self.core.

Copy link
Member

Choose a reason for hiding this comment

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

Okay! As long as we call this out in the "API changes" section of the release notes!

Copy link
Member Author

Choose a reason for hiding this comment

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

@john-science
Copy link
Member

To be clear, the reason I haven't tried to merge this is that we are currently in a code freeze. I am not ignoring you.

@albeanth
Copy link
Member Author

I wasn't assuming you were. There's a whole bunch of downstream PRs for this so yeah, it'll be a little while before we can make movement on this. Feel free to mark as draft if you want, or close it. I can always reopen it later.

@john-science
Copy link
Member

@albeanth This PR is getting a bit old; there are a bunch of conflicts.

Also, we should move the release note from 0.2.9 to 0.3.1.

@albeanth
Copy link
Member Author

This PR is getting a bit old; there are a bunch of conflicts.

Also, we should move the release note from 0.2.9 to 0.3.1.

@john-science ok, the merge conflicts have been resolved and this feature branch is now up to date with main

doc/release/0.2.rst Outdated Show resolved Hide resolved
@john-science john-science self-requested a review April 25, 2024 17:31
@albeanth albeanth merged commit 7b26f79 into terrapower:main Apr 25, 2024
11 checks passed
@albeanth albeanth deleted the rmBlockReactorRelation branch April 25, 2024 19:20
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.

None yet

2 participants