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.

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

Add torsiondrive compute procedure #305

Merged
merged 11 commits into from
Sep 9, 2021
Merged

Add torsiondrive compute procedure #305

merged 11 commits into from
Sep 9, 2021

Conversation

SimonBoothroyd
Copy link
Contributor

@SimonBoothroyd SimonBoothroyd commented Jul 5, 2021 •
edited by dotsdl
Loading

Description

This PR implements an initial proof of concept torsiondrive procedure as outlined in issue #306.

There are a number of questions about this PR:

  • As qcelemental does not have native models for torsion drives I created some here based off of the QCElemental optimisation models and the QCPortal torsion drive models. Are these something that could / should live in QCElemental?

  • Is it more efficient to run multiple torsion drives at once, each with access to one core, or one torsion drive at once using all of the cores.

If the QCE maintainers would be happy to accept a PR similar to this I'd be happy to add an example / tests here.

An example input would look like:

molecule = Molecule(
    symbols=["C", "C", "H", "H", "H", "H", "H", "H"],
    connectivity=[
        (0, 1, 1),
        (0, 2, 1),
        (0, 3, 1),
        (0, 4, 1),
        (1, 5, 1),
        (1, 6, 1),
        (1, 7, 1),
    ],
    geometry=[
        1.5403406836914142, -1.0173082391323545, 0.9312810207342496,
        4.0719763300123235, -0.0975682592642413, -0.02203578938791481,
        0.0002563605701713713, 0.0013953403968729996, 0.0011121160323317373,
        1.309831306165048, -3.03614919350581, 0.5491856718564878,
        1.3800394103640543, -0.7181256543708329, 2.9707878359388222,
        5.6120991748009645, -1.1161249890160694, 0.907991575289461,
        4.302418801484787, 1.921022388748466, 0.36057345099334853,
        4.232223312568672, -0.3961916040297606, -2.061588178357897]
)

result = compute_procedure(
    input_data=TorsionDriveInput(
        keywords={
            "dihedrals": [(2, 0, 1, 5)],
            "grid_spacing": [60]
        },
        input_specification=QCInputSpecification(
            driver=DriverEnum.gradient,
            model=Model(method="UFF", basis=None)
        ),
        initial_molecule=molecule,
        optimization_spec=OptimizationSpecification(
            procedure="geomeTRIC",
            keywords={
                "coordsys": "dlc",
                "maxiter": 300,
                "program": "rdkit",
            }
        )
    ),
    procedure="torsiondrive"
)

print(result)

Closed #306

Changelog description

Adds a new torsiondrive compute procedure

Status

  • Code base linted
  • Ready to go

@SimonBoothroyd SimonBoothroyd mentioned this pull request Jul 5, 2021
Closed
@codecov
Copy link

codecov bot commented Jul 5, 2021 •
edited
Loading

Codecov Report

Merging #305 (9a7b313) into master (e248d2f) will increase coverage by 0.19%.
The diff coverage is 94.44%.

@dotsdl
Copy link
Collaborator

dotsdl commented Jul 15, 2021

Looks great @SimonBoothroyd! Thank you for putting this together!

I think this covers all the bases, and I prefer the simplicity of running one optimization at a time with config settings indicating how many threads and how much memory is available to the program running the gradient calculations.

I think we should press forward with this. Would you like to create a test next?

@SimonBoothroyd SimonBoothroyd marked this pull request as ready for review July 19, 2021 10:53
@SimonBoothroyd
Copy link
Contributor Author

Thanks @dotsdl - tests added in 4a68fe9.

It looks like the failures are mostly un-related. I'm not sure why test_geometric_generic is now failing - my guess is that the two entries of this test have always been skipped due to missing dependencies, and now that the dependencies (i.e. geometric + openmm) are present the test is actually being run but failing due to bad expected values.

@dotsdl
Copy link
Collaborator

dotsdl commented Jul 21, 2021

Agree with you @SimonBoothroyd that the errors we see in the tests are unrelated to the changes introduced by this PR. I wouldn't consider those blocking for merge of this feature.

@loriab, is there anything additional you would like to see before merging this? This functionality simplifies execution of torsiondrives by QC* users substantially, and I'm eager to get it into the library!

loriab
loriab reviewed Jul 29, 2021
View reviewed changes
Copy link
Collaborator

@loriab loriab left a comment

Choose a reason for hiding this comment

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

This looks like a great start. It'd be good to have a torsiondrive procedure in QCEngine

  • as a model of what duplicate qcng procedure and qcf service looks like
  • because I plan to be migrating several more procedures into qcng in the next months and the more variety the less likely we'll make wrong/restrictive decisions
  • there's a lot of experience running torsiondrive so easier to head off wrong decisions on multispawn procedures

lmk if a zoom-like mtg would be useful, and thanks for the new incipient functionality!

qcengine/procedures/torsiondrive.py Outdated Show resolved Hide resolved
qcengine/procedures/torsiondrive.py Outdated Show resolved Hide resolved
qcengine/procedures/torsiondrive.py Outdated Show resolved Hide resolved
@SimonBoothroyd SimonBoothroyd mentioned this pull request Aug 3, 2021
Merged
2 tasks
@SimonBoothroyd
Copy link
Contributor Author

Thanks @loriab - I've made the changes you suggested and have opened up an initial PR for adding the models to QCElemental here MolSSI/QCElemental#268 where we can maybe discuss them in a bit more detail.

@loriab
Copy link
Collaborator

loriab commented Aug 27, 2021

qcel 0.22 ready with the models this PR needs

@SimonBoothroyd
Copy link
Contributor Author

Thanks @loriab ! These should be integrated in ff41b75.

As far as I can tell this should be G2G now (cc @dotsdl @bennybp)

@dotsdl
Copy link
Collaborator

dotsdl commented Sep 8, 2021

Performing final review with aim to merge.

dotsdl
dotsdl approved these changes Sep 9, 2021
View reviewed changes
Copy link
Collaborator

@dotsdl dotsdl left a comment

Choose a reason for hiding this comment

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

Looks great @SimonBoothroyd; incredible work! I have one question to address, but also ready to merge as-is!

qcengine/procedures/torsiondrive.py Outdated Show resolved Hide resolved
qcengine/procedures/torsiondrive.py Outdated Show resolved Hide resolved
@dotsdl dotsdl merged commit e827405 into MolSSI:master Sep 9, 2021
@dotsdl
Copy link
Collaborator

dotsdl commented Sep 9, 2021

Merged! Thank you @SimonBoothroyd for this!

@pavankum pavankum mentioned this pull request Jan 15, 2022
Merged
2 tasks
@xiki-tempula xiki-tempula mentioned this pull request Jan 16, 2022
Closed
@pavankum pavankum mentioned this pull request Mar 22, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotsdl dotsdl dotsdl approved these changes

@loriab loriab loriab left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SUGGESTION] Add a TorsionDrive procedure
3 participants
@SimonBoothroyd @dotsdl @loriab