Chemprop #1683
Chemprop #1683
Conversation
cgrambow
added
Topic: Thermo
Status: Ready for Review
|
Regarding environment files, my thought is that by the end of this, we will go back to having a single environment file for both mac and linux (and dropping the windows one) which will be I assume this breaks the Python 2 build? If so, then this will not be merged until the last stage of the transition. |
environment_py3.yml
Outdated
| @@ -1,15 +1,16 @@ | |||
| name: rmg_env | |||
| channels: | |||
| - pytorch | |||
There was a problem hiding this comment.
Could you put the pytorch channel on the bottom, so in case we ever do add a package to the RMG channel, it will supersede it?
There was a problem hiding this comment.
Oh actually, I just checked pytorch in default Anaconda and it seems they updated to the current version 5 days ago. I can test if the defaults build works correctly with chemprop and then could remove the pytorch channel completely.
There was a problem hiding this comment.
Removed pytorch channel and force pushed.
| @@ -1,6 +1,3 @@ | |||
| #!/usr/bin/env python | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
Did you remove all of the shebangs intentionally? Are they not necessary in Python 3?
There was a problem hiding this comment.
The coding specification is not necessary in Python 3 because that is now the default.
I removed the first line as well because the modules aren't meant to be run as scripts and Python doesn't need that line if you're importing a module. That line is only useful if you plan to run a Python file without telling the command line explicitly that you're using Python. Most RMG files have that first line but unless they're scripts, none of those files need it. We could consider removing it everywhere. Or would you rather I keep it here to be consistent with the rest of RMG?
There was a problem hiding this comment.
In that case, I think we can remove them everywhere. That can be a separate PR though.
rmgpy/ml/estimator.py
Outdated
| @@ -28,17 +25,24 @@ | |||
| # # | |||
| ############################################################################### | |||
|
|
|||
| from argparse import Namespace | |||
There was a problem hiding this comment.
Put from imports below imports
There was a problem hiding this comment.
Is that a PEP-8 rule or an RMG rule? I thought imports were supposed to be alphabetical, but I might be wrong.
There was a problem hiding this comment.
Great question! I thought it was in PEP-8 but apparently not. It seems that it's just the default PyCharm style.
I like grouping from and direct imports separately, but I would want to get everyone's opinions before making it an RMG rule.
There was a problem hiding this comment.
So in any case we want the following order for each module:
- standard library imports
- related third party imports
- local application/library specific imports
And then I think I agree with you that it makes sense to first have normal imports and then from imports and imports should be alphabetical within each group.
cgrambow
force-pushed
the
chemprop
branch
4 times, most recently
from
90a6466
to
c64f1ae
Compare
|
So it seems that even the pytorch builds on the official anaconda channel are not reliable. The only error-free builds are available on the pytorch channel. This is what was causing your issue, @mliu49. So I actually went ahead and added the pytorch channel back in. I also upgraded the chemprop build on the RMG channel to use pytorch-cpu, which is actually a lot smaller as well and doesn't install CUDA dependencies. Obviously this means that chemprop cannot be used with GPU but that should be fine for RMG users. Tested locally, on Mithrim, on RMG, and on Pharos. Added a bunch of fixup commits to address the issues. |
|
Thanks! I think the fixes look good. What led to the addition of hyperopt and tensorboardx as dependencies compared to before? |
|
They already were dependencies of chemprop before but when I built the environment locally I had conda-forge already in my .condarc so it didn't complain that it couldn't resolve those dependencies. |
rmgpy/ml/estimator.py
Outdated
|
|
||
| Returns: ThermoData | ||
| """ | ||
| smi = molecule.SMILES if isinstance(molecule, Molecule) else molecule |
There was a problem hiding this comment.
The cp0 and cpinf calculation below does not work if molecule is a string.
Also, could you update thermoTest.TestThermoDatabase.setUpClass?
There was a problem hiding this comment.
Done. Also rebased onto new py3.
Only use two models, one that predicts enthalpy and one that predicts entropy and heat capacity simultaneously. Uncertainty cannot be estimated with the new models. If chemprop is not available, don't raise error until trying to use it.
Packages requiring libgcc have been rebuilt to require libgcc-ng instead. With this the conda solver can resolve the environment without needing pydot-ng and pygpu to be given explicitly
Motivation or Problem
DDE does not work with Python 3.
Description of Changes
Switch the ML thermo estimator to chemprop. New models have been added in database PR ReactionMechanismGenerator/RMG-database#351.
Testing
Once Python 3 transition has occurred, check that tests pass and ML example can be run.
Question for Reviewer
I don't understand how I should be editing the different environment files. I added chemprop to the py3 environment file. Should I also add it to the other ones?