-
Notifications
You must be signed in to change notification settings - Fork 66
OpenMC depletion coupling #921
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
base: devel
Are you sure you want to change the base?
Conversation
|
FYI @aprilnovak and @pshriwise; still a mess but it works! |
|
Job Documentation on 5e3baba wanted to post the following: View the site here This comment will be updated on new commits. |
95b5008 to
5e3baba
Compare
pshriwise
left a comment
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.
This looks really good!! Thank you for creating this @loganharbour. Mostly line changes here and a couple questions about integrating it into a Cardinal install.
python/cardinal_operator.py
Outdated
| super().__init__(*args, **kwargs) | ||
|
|
||
| self._control = None | ||
| self.cardinal_cmd = None |
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.
Let's move these attribute initializations before the super().__init__ call so we don't get an AttributeError from CardinalOperator.__del__ if the parent constructor fails.
| super().__init__(*args, **kwargs) | |
| self._control = None | |
| self.cardinal_cmd = None | |
| self._control = None | |
| self.cardinal_cmd = None | |
| super().__init__(*args, **kwargs) |
| nuclides = [n for n in t.nuclides] | ||
| names_path = f'{uo_path}/names' | ||
| self._control.setControllableVectorString(names_path, nuclides) | ||
| break |
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.
We can remove this now, right?
| break |
| if self._control is None: | ||
| self.start_cardinal() |
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.
Suggesting the tiniest of refactors: This pattern appears often. Do we want to put it into a method like ensure_cardinal?
|
|
||
| # All of these imports are awful and we'll figure | ||
| # out a better way to do it before merging | ||
| try: |
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.
It seems fair to me to expect users to have installed the OpenMC Python package used with Cardinal, but it doesn't look like your typical installation as that's going to occur from the submodule. Offhand thoughts:
- (optionally) Install the OpenMC Python API as part of the Cardinal build
- Include setting of
PYTHONPATHto the OpenMC submodule in Cardinal's installation docs.
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.
It'd be awesome if we automatically installed the Python API when installing Cardinal -- it's listed in this issue: #592
| try: | ||
| from cardinal_operator import CardinalOperator | ||
| except: | ||
| cardinal_contrib = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', '..', 'python')) |
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.
see above, this could also be solved by requiring the PYTHONPATH to be set to CARDINAL_HOME/python in the installation notes.
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.
A warning similar to the one about NEKRS_HOME could be provided perhaps.
08524b4 to
470411b
Compare
|
Job Documentation, step Sync to remote on 791f7ac wanted to post the following: View the site here This comment will be updated on new commits. |
|
@loganharbour would you please rebase this off |
aa7581f to
ea9034b
Compare
|
Looks like we'll need the |
A bit. I've needed to find a way to allow custom installs for apps. I'll get to that tomorrow |
|
I'm trying to run this locally so I can learn how to use this feature. Do I need to pip install the new python code? Is it as simple as? |
|
|
Actually I think I got a bit farther now. I tried setting I think this is probably due to some source code changes in Cardinal which have happened over the last couple of months from the new |
|
This test case seems to have originally been working after the |
This is strange to me b/c the |
It appears as if |
|
Ah, I know what's going on. |
Ah good catch! |
Co-authored-by: Kevin Sawatzky <66632997+nuclearkevin@users.noreply.github.com> Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>
|
A couple of Python modules missing in the test env it seems ( |
Yep. Is that it? |
I think so. Thoughts on using The |
|
Trying to run this still - getting farther, but hitting this: @loganharbour are there some files missing from this PR, or something needs to be updated still? |
|
It looks like arguments aren't being passed right to |
|
Can you help with debugging? I'm not sure on how these files are meant to look since I think this is the only example case. |
Things still to do
depletion.pyCardinalOperatorthat checks to see if MOOSE is also done, otherwise error