-
Notifications
You must be signed in to change notification settings - Fork 454
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
Various fixes to Python API and scripts #378
Conversation
…emove statepoint_cmp.py and statepoint_meshplot.py scripts since h5diff can be used in place of the former and plot_mesh_tally can be used in place of the latter.
…_restart.py to utils/openmc folder to make it part of Python API.
I should also mention that I updated statepoint.py to work with Python 3. |
Sure, I will take a look at this in the morning. |
@@ -7,6 +8,9 @@ | |||
import openmc | |||
from openmc.constants import * | |||
|
|||
if sys.version > '3': |
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.
Perhaps we should make Python 3 the standard for Travis? Although I use Python 3 from time-to-time, I still haven't made it the default on my systems, hence issues like this arise. Anyway, just a thought.
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.
For the record, I use Python 3 exclusively these days. So you have at least one user :). Adding Python 3 to Travis would probably help.
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.
There are always the cool kids and a few that are slower than the rest of the pack in every class.
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.
haha <insert rant about arch linux here>
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'm inclined to leave Travis using 2.7 for the time being. Most (all except Arch?) distributions use 2 as default still. @scopatz do you have any thoughts about good ways to test both 2 and 3?
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.
If you add both 2.7 and 3.4 to the Python list in the travis.yml file, it will test both languages in parallel. This is what pyne and cyclus do. See https://github.com/pyne/pyne/blob/develop/.travis.yml#L2 Should be a one or two line change,
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.
Didn't realize it was that simple - thanks!
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.
Yep, not hard and totally worth it :)
This PR looks good to me save for my few very minor comments. |
Everything looks good. Thanks for the fixes @paulromano |
Various fixes to Python API and scripts
A summary of the changes in this pull request:
@wbinventor can you review?