-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fix python2 incompatibility #138
Conversation
@rjleveque I think this fixes everything but can you check when you get a chance and then retry the GeoClaw PR? |
It seems to be running now, stay tuned... However, when I first tried to run it I was missing an fgmax input file in this directory and so it threw a fortran error, and several Python errors in the process of doing so. Maybe this is the expected behavior in this case, but it may make it harder for the user to figure out what the real error was. Is it possible to print out a more useful message saying the fortran code died rather than looking like a Python error in this case? (Note to self: change
|
except subprocess.CalledProcessError as cpe: | ||
raise ClawExeError('error', cpe.returncode, cpe.cmd, | ||
output=cpe.output, | ||
stderr=cpe.stderr) from cpe |
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 think the from cpe
just keeps the existing call stack when raising the error, so potentially helps a bit with debugging. Is that a python 3-only syntax? Or is there another reason you dropped that?
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.
As far as I know this was added to Python 3.x unfortunately.
That's a good idea Randy. We can check to see if the Fortran returned a non-zero code I think and make that more clear. |
I added print("")
print("*** FORTRAN EXE FAILED ***")
print("") before the re-raise of the |
I think this makes sense, but is there any reason not to replace the text of the error (currently just |
Right now it's printed above the exception. I think the real issue is that the traceback is the last thing printed rather than the error but I am not sure how to get around this. |
It's worth remembering that Numpy and Matplotlib are committed to dropping Python 2 support by Jan. 1, 2020. This will essentially force us to do the same. |
It looks like the 2nd to last thing is |
That's a good idea. Just modified the code. |
Works for me and ok to merge as far as I'm concerned. I like having it print something at the end pointing to the fortran error. I also just fixed the problem in |
This addresses Python 2.x incompatibilities in PR #136 and includes #137 fixes.