-
Notifications
You must be signed in to change notification settings - Fork 94
Moving Spent Fuel pool off Core and onto Reactor #1336
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
Conversation
@john-science does this resolve and close #876 ? |
@@ -466,7 +463,8 @@ def removeAssembly(self, a1, discharge=True): | |||
self.remove(a1) | |||
|
|||
if discharge and self._trackAssems: | |||
self.sfp.add(a1) | |||
if hasattr(self.parent, "sfp"): | |||
self.parent.sfp.add(a1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be an else
for the case when this conditional fails? I don't expect it to for standard analyses, but there might be some corner case where a user inits a standalone Core
and does stuff with it. In that case, they'd be thinking they're adding assemblies to the sfp, but really wouldn't and there wouldn't be any way of knowing so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, imagine we said:
if self.parent is not None and hasattr(self.parent, "sfp"):
It turns out hasattr()
covers our bases so we don't need the "is not None" statement.
My major concerns are:
- This is Python, you can build a "Core" object in vaccuum with no "Reactor" object.
- Maybe someone wants to build multiple cores for an analysis, that should be allowable with them having to build multiple reactors.
a. People who complain about ARMI usually complain that everything is too complicated because they can't just do the one thing they want without: building a whole reactor or building an Operator/Plugin. So I take it people would prefer they were freed from having to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally. I wasn't necessarily concerned with an is not None
getting checked. I was thinking more about what happens in the case when there isn't a parent Reactor
but a user still tries to add an assembly to r.sfp
. In this current implementation, the user would never know that the assembly wasn't getting added as the code wouldn't tell them. The conditional would just evaluate to false and move on. So I was thinking maybe something akin to:
if hasattr(self.parent, "sfp"):
self.parent.sfp.add(a1)
else:
runLog.warning(f"There is no parent Reactor object for {self}, so {a1} will not be added to the spent fuel pool")
I believe @keckler had a similar thought as well. #1336 (comment)
@@ -527,7 +525,8 @@ def removeAllAssemblies(self, discharge=True): | |||
assems = set(self) | |||
for a in assems: | |||
self.removeAssembly(a, discharge) | |||
self.sfp.removeAll() | |||
if hasattr(self.parent, "sfp"): | |||
self.parent.sfp.removeAll() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here on else
for failing conditional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything here that conflicts with my models which employ the SFP, so I'm good with these changes!
What is the change?
I moved the Spent Fuel Pool (SFP) object was on the
Core
instead of theReactor
.I wanted to make the shift more cleanly than I did, but when you call
Core.removeAssembly()
, it wants to put the assembly in the SFP. Which means theCore
needs to at least know the SFP is on theReactor
. But fine.For some reason the
Why is the change being made?
The SPF is, objectively, not inside the Core. This was just wrong. And more importantly, counterintuitive for the users.
Checklist
doc/release/0.X.rst
) are up-to-date with any important changes.doc
folder.setup.py
.