-
Notifications
You must be signed in to change notification settings - Fork 12
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 non-interactive backend in nwl utils to avoid matplotlib cubit conflict #148
Conversation
We should perhaps consider including OpenMC v0.15.0 in the environment YAML file |
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 good to me. I'll let @Edgar-21 and @connoramoreno make the final call on whether to add the OpenMC version to the environment before merge.
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.
Just a small question about removing the multiprocessing
module from the list of imports.
@@ -5,8 +5,10 @@ | |||
import pystell.read_vmec as read_vmec | |||
import matplotlib.pyplot as plt | |||
import concurrent.futures | |||
from multiprocessing import cpu_count |
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.
Why are we removing multiprocessing
? Wouldn't that remove parallelization from the NWL routine?
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.
concurrent.futures
is what does the multiprocesssing, I originally attempted to make some guess about how many cpus to use based on cpu_count
but ended up removing that bit, but this import remained
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.
Oh I see! We're good then. Let's just figure out whether we want to include OpenMC as a dependency now...
I'm in favor of at least including some mention that OpenMC v0.15.0 is needed to use the NWL utility. I'm not opposed to including it as a hard dependency to prevent user error on that front, although I recognize it complicates the environment and may become a problem in the future, like it was in the past. |
Looks good, thanks @Edgar-21 |
With the newest version of openmc (0.15.0) the previous conflict with Cubit seems to have been resolved. The remaining conflict with matplotlib can be resolved as in this PR, allowing the full NWL workflow to be performed in one script. In a future PR the nwl_utils example could be updated to be a single script.