Skip to content
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

Merged
merged 4 commits into from Dec 4, 2019
Merged

Conversation

ajocher
Copy link
Contributor

@ajocher ajocher commented Oct 15, 2019

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.

Copy link
Contributor

@mliu49 mliu49 left a 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.

Comment on lines 7 to 12
from __future__ import division
from __future__ import print_function

from builtins import zip
from builtins import range
from past.utils import old_div
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

from rmgpy.chemkin import loadChemkinFile, getSpeciesIdentifier

from builtins import str
from builtins import range
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -48,6 +48,7 @@
to
<policy domain="coder" rights="read|write" pattern="EPS" />
"""
from __future__ import print_function
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@mliu49 mliu49 left a 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",
Copy link
Contributor

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(), ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -48,7 +48,6 @@
to
<policy domain="coder" rights="read|write" pattern="EPS" />
"""

Copy link
Contributor

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.

Copy link
Contributor Author

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",
Copy link
Contributor

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.

Copy link
Contributor Author

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",
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ajocher
Copy link
Contributor Author

ajocher commented Dec 3, 2019

Thanks for the comments! I squashed and rebased.

@mliu49 mliu49 merged commit e409c4b into master Dec 4, 2019
@mliu49 mliu49 deleted the rmg2to3scripts branch December 4, 2019 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants