-
Notifications
You must be signed in to change notification settings - Fork 145
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
Update RMG-database scripts and notebooks to Python 3 #364
Conversation
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 made a couple quick comments about imports which are not necessary since we aren't aiming for compatibility with both Python 2 and Python 3.
scripts/evansPolanyi.py
Outdated
from __future__ import division | ||
from __future__ import print_function | ||
|
||
from builtins import zip | ||
from builtins import range | ||
from past.utils import old_div |
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.
Since we're going for Python 3 compatibility only, we don't need these compatibility measures.
__future__
imports are not necessary since we are using Python 3 only- For division, we should use the built-in
/
operator instead of importing old_div. - We can use zip and range from the default Python 3 libraries instead of the
builtins
module, so these imports are not necessary.
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.
done
scripts/importChemkinLibrary.py
Outdated
from rmgpy.chemkin import loadChemkinFile, getSpeciesIdentifier | ||
|
||
from builtins import str | ||
from builtins import range |
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.
These imports are also not necessary.
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.
done
scripts/process_family_images.py
Outdated
@@ -48,6 +48,7 @@ | |||
to | |||
<policy domain="coder" rights="read|write" pattern="EPS" /> | |||
""" | |||
from __future__ import print_function |
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 don't need this import.
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.
done
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 made a couple more minor comments. I think once you fix those, you can squash and rebase.
" mydict = dict(zip(index_list, kvals))\n", | ||
"\n", | ||
" # Find key and value of max rate coefficient\n", | ||
" key_max_rate = max(mydict.iteritems(), key=operator.itemgetter(1))[0]\n", | ||
" key_max_rate = max(iter(mydict.items()), key=operator.itemgetter(1))[0]\n", |
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.
Could you change this to just max(mydict.items(), ...
?
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.
Done.
scripts/process_family_images.py
Outdated
@@ -48,7 +48,6 @@ | |||
to | |||
<policy domain="coder" rights="read|write" pattern="EPS" /> | |||
""" | |||
|
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 think this change is necessary if there are no other changes to this 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 commit is removed.
"reaction_dict = {}\n", | ||
"for library_name in libraries:\n", | ||
" kinetic_library = database.kinetics.libraries[library_name]\n", | ||
" for index, entry in iter(kinetic_library.entries.items()):\n", |
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 can also be kinetics_library.entries.items()
without iter
.
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.
Done.
" for index, entry in kineticLibrary.entries.iteritems():\n", | ||
"for library_name in libraries:\n", | ||
" kinetic_library = database.kinetics.libraries[library_name]\n", | ||
" for index, entry in iter(kinetic_library.entries.items()):\n", |
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 thing here with iter
.
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.
Done.
ca156a7
to
7f5fa5c
Compare
Thanks for the comments! I squashed and rebased. |
This pull request addresses "Update RMG-database scripts and notebooks" in ReactionMechanismGenerator/RMG-Py#1726.
In addition, RMG-Java related scripts and unnecessary scripts are deleted as discussed in #363.
generateFilterArrheniusFits.ipynb is updated but can only be finalized once from rmgpy/data/kinetics/database.py has 'filter_limit_fits' updated (currently not in master and Python 3).
Errors related to drawing a molecule are fixed in ReactionMechanismGenerator/RMG-Py#1735.
The documentation is updated in ReactionMechanismGenerator/RMG-Py#1767.