Skip to content

Create OFF Mols from QCArchive #472

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 35 commits into from
Feb 6, 2020
Merged

Create OFF Mols from QCArchive #472

merged 35 commits into from
Feb 6, 2020

Conversation

@jthorton
Copy link
Collaborator

@jthorton jthorton commented Dec 11, 2019 •
edited
Loading

@j-wags j-wags changed the title [WIP] Creat OFF Mols from QCArchive [WIP] Create OFF Mols from QCArchive Dec 11, 2019
@codecov-io
Copy link

codecov-io commented Dec 11, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@5ae3dbc). Click here to learn what that means.
The diff coverage is 91.89%.

Allowed RDKit to read PDBs only when combined with a valid smiles str…
a00f878 
…ing, this adds a new classmethod `from_pdb`.

This introduces new functionality for `are_isomorphic` function which now can return an atom_mapping and can match based on user criteria.
This will be useful to refactor `Topology.from_openmm`.
@lgtm-com
Copy link

lgtm-com bot commented Dec 17, 2019

This pull request introduces 1 alert when merging a00f878 into e3a8856 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

j-wags
j-wags reviewed Dec 17, 2019
View reviewed changes
j-wags
j-wags reviewed Dec 17, 2019
View reviewed changes
jchodera
jchodera reviewed Dec 18, 2019
View reviewed changes
jchodera
jchodera reviewed Dec 18, 2019
View reviewed changes
jchodera
jchodera reviewed Dec 18, 2019
View reviewed changes
jchodera
jchodera reviewed Dec 18, 2019
View reviewed changes
Allowed RDKit to read PDBs only when combined with a valid smiles str…
fea5afd 
…ing, this adds a new classmethod `from_pdb`.

This introduces new functionality for `are_isomorphic` function which now can return an atom_mapping and can match based on user criteria.
This will be useful to refactor `Topology.from_openmm`.

hill_formula implemented
@lgtm-com
Copy link

lgtm-com bot commented Dec 18, 2019

This pull request introduces 1 alert when merging fea5afd into e3a8856 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

Allowed RDKit to read PDBs only when combined with a valid smiles str…
22ebb1d 
…ing, this adds a new classmethod `from_pdb`.

This introduces new functionality for `are_isomorphic` function which now can return an atom_mapping and can match based on user criteria.
This will be useful to refactor `Topology.from_openmm`.

hill_formula implemented

to_qcshema implemented
from mapped_smiles implemented with internal atom ordering remapping.

from qcshema started from cmiles only using an instance.
@lgtm-com
Copy link

lgtm-com bot commented Dec 19, 2019

This pull request introduces 2 alerts when merging 22ebb1d into e3a8856 - view on LGTM.com

new alerts:

  • 1 for Unreachable code
  • 1 for Variable defined multiple times

jthorton
jthorton commented Dec 19, 2019
View reviewed changes
jthorton
jthorton commented Dec 19, 2019
View reviewed changes
j-wags
j-wags reviewed Dec 19, 2019
View reviewed changes
ChayaSt
ChayaSt reviewed Dec 19, 2019
View reviewed changes
joshhorton added 3 commits January 3, 2020 11:02
fixed __eq__, added the is_ismorphic_with(other) function
5e56a4d
Merge branch 'master' of https://github.com/openforcefield/openforcef…
50f06c5 
…ield into mol_from_qca
added tests for hill formula, is_isomorphic_with and are_isomorphic, …
73cc43c 
…and remaping tests. Also tweaked the toolkits to get the atom mapping if found and attach it to the moelecule properties dict.
@lgtm-com
Copy link

lgtm-com bot commented Jan 3, 2020

This pull request introduces 1 alert when merging 73cc43c into 7858706 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

added tests for hill formula, is_isomorphic_with and are_isomorphic, …
1d77151 
…and remaping tests. Also tweaked the toolkits to get the atom mapping if found and attach it to the moelecule properties dict.

envirmoent update for testing
@lgtm-com
Copy link

lgtm-com bot commented Jan 3, 2020

This pull request introduces 1 alert when merging 1d77151 into 7858706 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

j-wags
j-wags reviewed Jan 3, 2020
View reviewed changes
j-wags
j-wags reviewed Jan 3, 2020
View reviewed changes
j-wags
j-wags reviewed Jan 3, 2020
View reviewed changes
j-wags
j-wags reviewed Jan 3, 2020
View reviewed changes
j-wags
j-wags reviewed Jan 3, 2020
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

This pull request introduces 2 alerts when merging 930ec61 into 5d97594 - view on LGTM.com

new alerts:

  • 1 for Non-iterable used in for loop
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

This pull request introduces 2 alerts when merging 4d6d195 into 5d97594 - view on LGTM.com

new alerts:

  • 1 for Non-iterable used in for loop
  • 1 for Module is imported with 'import' and 'import from'

@j-wags j-wags changed the title [WIP] Create OFF Mols from QCArchive Create OFF Mols from QCArchive Jan 31, 2020
@j-wags j-wags self-requested a review January 31, 2020 20:27
@j-wags j-wags self-assigned this Jan 31, 2020
@trevorgokey trevorgokey mentioned this pull request Feb 1, 2020
Closed
11 tasks
@lgtm-com
Copy link

lgtm-com bot commented Feb 2, 2020

This pull request introduces 2 alerts when merging 8291506 into 93929c4 - view on LGTM.com

new alerts:

  • 1 for Non-iterable used in for loop
  • 1 for Module is imported with 'import' and 'import from'

j-wags
j-wags requested changes Feb 2, 2020
View reviewed changes
Copy link
Member

@j-wags j-wags left a comment •
edited by jthorton
Loading

Choose a reason for hiding this comment

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

I am really excited to bring in this functionality, and I didn't realize how much progress you had made until I reviewed the code in full today. This is a massive addition, and you've added some great tests and docs.

Roughly half of the comments here are spelling changes, which you should be able to directly accept (or reject if you don't like them). I've made several suggestions and little comments that aren't blocking, but may be easily resolvable in case I just misunderstood something.

There are only two blocking issues:

  • The notebook shows a molecule energy of 6.6e22. I don't know if this is a unit-conversion thing or a misinterpreted geometry. Let's either fix this or add a comment to the notebook about what unit this is in.
  • I feel strongly that we should make from_mapped_smiles private (_from_mapped_smiles) until we resolve #412 and discuss the extent to which we want to support an atom_map property

Also, we shouldn't merge until all builds on Travis pass. As you pointed out earlier, the notebook tests will fail in the non-RDKit build. Either you or I can fix that by modifying the .travis.yml file to skip that if RDKit isn't installed. Happy to screenshare and do this after the Monday core-devs call

Also, we should update the release notes to talk about all this great new stuff you've added. Writing release notes is great -- it's kind of like writing a discussion section of a paper -- because you get to summarize all the conceptual stuff that you're now an expert in, and talk about how you made different decisions. That can be a separate pre-release PR, and I'm happy to do it if you aren't interested.

@lgtm-com
Copy link

lgtm-com bot commented Feb 3, 2020

This pull request introduces 2 alerts when merging 11fd6f1 into 93929c4 - view on LGTM.com

new alerts:

  • 1 for Non-iterable used in for loop
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Feb 3, 2020

This pull request introduces 2 alerts when merging d5649b8 into 93929c4 - view on LGTM.com

new alerts:

  • 1 for Non-iterable used in for loop
  • 1 for Module is imported with 'import' and 'import from'

@j-wags j-wags self-requested a review February 4, 2020 19:59
j-wags
j-wags approved these changes Feb 4, 2020
View reviewed changes
Copy link
Member

@j-wags j-wags left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Will be busy for the next few days, so I'm approving this now. Feel free to merge when

  • the notebook energy thing is resolved
  • tests pass
  • LGTM stops complaining

joshhorton added 2 commits February 5, 2020 10:30
Fixed notebook energy example using qcengine, added spelling changes …
5ae3dbc 
…and made canonical order tests match between toolkits.
Merge branch 'master' of https://github.com/openforcefield/openforcef…
38d8022 
…ield into mol_from_qca

Conflicts:
	openforcefield/topology/molecule.py
@jthorton
Copy link
Collaborator Author

jthorton commented Feb 5, 2020

The large notebook energy was due to an incorrect unit conversion in QCEngine which will be fixed with this PR, for now, I use RDKit and explain what units the energy is returned in. One of the LGTM errors about the Non-iterable is not something I have changed so not sure what is going on there. Just waiting on tests now.

@lgtm-com
Copy link

lgtm-com bot commented Feb 5, 2020

This pull request introduces 1 alert and fixes 1 when merging 65a1073 into 417886f - view on LGTM.com

new alerts:

  • 1 for Non-iterable used in for loop

fixed alerts:

  • 1 for Suspicious unused loop iteration variable

@jthorton
Copy link
Collaborator Author

jthorton commented Feb 5, 2020

Any idea why LGTM JavaScript is failing? @j-wags

@lgtm-com
Copy link

lgtm-com bot commented Feb 5, 2020

This pull request introduces 1 alert and fixes 1 when merging 83da5e5 into 417886f - view on LGTM.com

new alerts:

  • 1 for Non-iterable used in for loop

fixed alerts:

  • 1 for Suspicious unused loop iteration variable

@j-wags
Copy link
Member

j-wags commented Feb 6, 2020

No clue. We don't have Javascript AFAIK. This is a LGTM problem. Feel free to merge, this is not blocking.

@jthorton jthorton merged commit ee0f715 into master Feb 6, 2020
@j-wags j-wags added this to the 0.7.0 milestone Feb 14, 2020
@j-wags j-wags deleted the mol_from_qca branch February 18, 2020 02:57
@j-wags j-wags mentioned this pull request Mar 31, 2021
Merged
5 tasks
@j-wags j-wags mentioned this pull request Sep 29, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jchodera jchodera jchodera left review comments

@trevorgokey trevorgokey trevorgokey left review comments

@j-wags j-wags j-wags approved these changes

+1 more reviewer

@ChayaSt ChayaSt ChayaSt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@j-wags j-wags

Labels

None yet

Projects

None yet

Milestone

0.7.0

Development

Successfully merging this pull request may close these issues.

Add functionality to create OFFMol from QCAMol Add functionality to create OFFMols with specific indexing RDMol stereochemistry detector too strict

8 participants

@jthorton @codecov-io @j-wags @davidlmobley @jchodera @ChayaSt @trevorgokey