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

Allow ARCSpecies to be initialized with a conformer list file #104

Merged
merged 12 commits into from
Mar 28, 2019
Merged

Conversation

alongd
Copy link
Member

@alongd alongd commented Mar 25, 2019

Users can now initialize an ARCSpecies object with either of the conformers_before_optimization.txt or conformers_after_optimization.txt files. These files are automatically generated by ARC and saved in the Species 'geometry' folder. The latter also reports the conformer energies.

@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #104 into master will decrease coverage by 0.07%.
The diff coverage is 63.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage   40.62%   40.55%   -0.08%     
==========================================
  Files          22       22              
  Lines        5226     5265      +39     
  Branches     1357     1373      +16     
==========================================
+ Hits         2123     2135      +12     
- Misses       2760     2780      +20     
- Partials      343      350       +7
Impacted Files Coverage Δ
arc/job/submit.py 100% <ø> (ø) ⬆️
arc/job/job.py 20.36% <0%> (-1.7%) ⬇️
arc/species/converter.py 71.79% <0%> (ø) ⬆️
arc/main.py 42.2% <100%> (+0.16%) ⬆️
arc/scheduler.py 19.34% <57.14%> (-0.16%) ⬇️
arc/species/species.py 58.95% <77.41%> (+0.54%) ⬆️
arc/reaction.py 42.12% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b5db7a...1a8b806. Read the comment docs.

Copy link
Contributor

@oscarwumit oscarwumit left a comment

Choose a reason for hiding this comment

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

The code looks good. During testing, I find some minor issues:

[A]: The conformers_path argument works well with the ARCSpecies. However, if a user defines the species using an RMG object, such as:

spc = Species(molecule=[Molecule().fromSMILES("C=C[O]")], conformers_path = 'path')

Then a TypeError will occur:

__init__() got an unexpected keyword argument `conformers_path`

Possible solutions:

(1) add 'conformers_path' as a keyword argument to init in rmgpy/species.so

OR (2) add an exception in ARC to notify the users that they have to use ARCSpecies if they want to use conformers_path.


[B]: Currently an AttributeError will occur (related to PR 106) if a user specifies conformers_path as an argument without specifying a smile/adj list. For instance:

spc = ARCSpecies(label='methylamine', conformers_path = 'path')

Will give:

'NoneType' object has no attribute 'enumerate_bonds'

But the following will work

 spc = ARCSpecies(label='methylamine', conformers_path = 'path', **smiles = 'CN'**)

Suggestion: we can temporarily get around with this issue by specifying smiles/adj list along with the conformers_path. We should address the issue raised in PR 106, and also figure out why from_xyz does not work as expected.

Many thanks for your contribution. This feature is really useful. Please let me know if you have any questions. Great work 👍

@alongd
Copy link
Member Author

alongd commented Mar 27, 2019

Thanks!
Regarding point A, the way to define an ARCSpecies with an RMG Species might be:

rmg_spc = Species(molecule=[Molecule().fromSMILES("C=C[O]")])
arc_spc = ARCSpecies(rmg_species=rmg_spc, conformers_path='path/to/conformers.txt')

I wouldn't want to add conformers_path to the RMG species, since this info isn't being used in the RMG object.

Will #106 solve point B?

@oscarwumit
Copy link
Contributor

@alongd I just tested that PR #106 will NOT resolve point B. This problem is mentioned in issue #107. Otherwise, this PR works as expected. I think we should merge this PR, and address #107 in a separate PR.

oscarwumit
oscarwumit previously approved these changes Mar 28, 2019
alongd added 9 commits March 28, 2019 10:06
Instead of duplicate code for saving the 'before' and 'after' conformer
files
Also tests the Scheduler.save_conformers_file() method
Also made the required changes to schedulerTests
Don't generate coformers if their xyzs are available
Don't calculate conformers energies if energies already given
To differentiate between cases where the energies are given in a
`conformers_after_optimization.txt` file with all energies set to 0.

Also set Species.opt_level default value to None
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.

2 participants