Skip to content

Fix Espaloma system/template generator #271

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
Closed
kntkb wants to merge 12 commits into from
Closed

Fix Espaloma system/template generator #271

kntkb wants to merge 12 commits into from

Conversation

@kntkb
Copy link
Contributor

@kntkb kntkb commented May 22, 2023 •
edited
Loading

This draft PR is fixes #245 and #263 by adding template_generator_kwargs to the system generators.

ToDo

  • Add tests

@kntkb
Copy link
Contributor Author

kntkb commented Jun 13, 2023

I think EspalomaTemplateGenerator.generate_residue_template will assign nn partial charges (espaloma charge) when the molecule is passed to espaloma_model. This means that partial charges assigned to a molecule will be overwritten. If we want to keep and use the original charges assigned to the molecule, we will need to book keep the information and reassign it after calling espaloma_model.

@kntkb kntkb mentioned this pull request Jun 14, 2023
Closed
ijpulidos
ijpulidos reviewed Jun 15, 2023
View reviewed changes
openmmforcefields/generators/template_generators.py Outdated
Comment on lines 1729 to 1730
# NOTE: espaloma (nn) partial charges are assigned to molecules automatically if available.
# We need to overwrite the partial charges if we want to read them from the molecule.
Copy link
Collaborator

@ijpulidos ijpulidos Jun 15, 2023

Choose a reason for hiding this comment

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

Good catch! We need to make sure if this is the case because we want users to be able to use charges from other methods/models combined with espaloma. I'll raise an issue about this so we can keep track of this.

@ijpulidos ijpulidos mentioned this pull request Jun 15, 2023
Closed
@kntkb
Copy link
Contributor Author

kntkb commented Jun 20, 2023

I think EspalomaTemplateGenerator.generate_residue_template will assign nn partial charges (espaloma charge) when the molecule is passed to espaloma_model. This means that partial charges assigned to a molecule will be overwritten. If we want to keep and use the original charges assigned to the molecule, we will need to book keep the information and reassign it after calling espaloma_model.

This is now supported in c198793

@ijpulidos ijpulidos mentioned this pull request Jul 7, 2023
Closed
@ijpulidos ijpulidos added this to the 0.11.3 milestone Jul 7, 2023
@ijpulidos ijpulidos self-assigned this Jul 11, 2023
@ijpulidos
Copy link
Collaborator

ijpulidos commented Jul 11, 2023

We need to make sure we respect this #282 (comment)

@ijpulidos ijpulidos changed the title Fix/system template generator Fix Espaloma system/template generator Jul 19, 2023
@ijpulidos ijpulidos mentioned this pull request Jul 19, 2023
Merged
1 task
@kntkb
Copy link
Contributor Author

kntkb commented Sep 22, 2023

@ijpulidos Is it OK to close this PR since we have a #293 that replaces this purpose?

@ijpulidos
Copy link
Collaborator

ijpulidos commented Sep 22, 2023

Yes, closing!

@ijpulidos ijpulidos closed this Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ijpulidos ijpulidos ijpulidos left review comments

Assignees

@ijpulidos ijpulidos

Labels

None yet

Projects

None yet

Milestone

0.11.3

Development

Successfully merging this pull request may close these issues.

[Enhancement] Add options to EspalomaTemplateGenerator to deploy different espaloma models

2 participants

@kntkb @ijpulidos