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

Create RestCapableHybridTopologyFactory #848

Merged
merged 106 commits into from
Apr 13, 2022
Merged

Create RestCapableHybridTopologyFactory #848

merged 106 commits into from
Apr 13, 2022

Conversation

zhang-ivy
Copy link
Contributor

@zhang-ivy zhang-ivy commented Aug 20, 2021

Description

A subclass of HybridTopologyFactory that handles:
- alchemical modification of atoms
- REST scaling
- 4th dimension softcore
with custom forces. Supports one alchemical region and one REST region.

See docstring of RestCapablePMEHybridTopologyFactory for more details.

Motivation and context

Resolves #???

How has this been tested?

Change log


@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #848 (7cae422) into main (d3649d1) will increase coverage by 2.59%.
The diff coverage is 83.48%.

@ijpulidos
Copy link
Contributor

CC #873 and port back to this when it works

@ijpulidos
Copy link
Contributor

@jchodera Your feedback is welcomed here! :)

@mikemhenry
Copy link
Contributor

@zhang-ivy
Copy link
Contributor Author

zhang-ivy commented Mar 21, 2022

As discussed, to fix the tests with current OpenMM versions. We could support current/previous versions of Openmm by checking the addComputedValue attribute and use it if available. We can use hasattr(CustomNonbondedForce, "addComputedValue") for this purpose.

Done!

The tests are now failing because pytest is not defined? @mikemhenry @ijpulidos

==================================== ERRORS ====================================
________________ ERROR collecting perses/tests/test_relative.py ________________
perses/tests/test_relative.py:1041: in <module>
    @pytest.mark.gpu_needed
E   NameError: name 'pytest' is not defined
________________ ERROR collecting perses/tests/test_relative.py ________________
perses/tests/test_relative.py:1041: in <module>
    @pytest.mark.gpu_needed
E   NameError: name 'pytest' is not defined

@mikemhenry
Copy link
Contributor

As discussed, to fix the tests with current OpenMM versions. We could support current/previous versions of Openmm by checking the addComputedValue attribute and use it if available. We can use hasattr(CustomNonbondedForce, "addComputedValue") for this purpose.

Done!

The tests are now failing because pytest is not defined? @mikemhenry @ijpulidos

==================================== ERRORS ====================================
________________ ERROR collecting perses/tests/test_relative.py ________________
perses/tests/test_relative.py:1041: in <module>
    @pytest.mark.gpu_needed
E   NameError: name 'pytest' is not defined
________________ ERROR collecting perses/tests/test_relative.py ________________
perses/tests/test_relative.py:1041: in <module>
    @pytest.mark.gpu_needed
E   NameError: name 'pytest' is not defined

I think I fixed this! I actually introduced this error when I marked the test to run on the GPU, sorry!

@zhang-ivy
Copy link
Contributor Author

@mikemhenry : No worries, thanks!

@zhang-ivy
Copy link
Contributor Author

@mikemhenry : I think the only thing this PR is missing is addressing #899

@mikemhenry
Copy link
Contributor

@zhang-ivy So sorry! I didn't realize you were still working on this!! Sorry about that merge commit, I hope I did it right!

@zhang-ivy
Copy link
Contributor Author

zhang-ivy commented Mar 29, 2022

@mikemhenry : No worries! I just realized I forgot something, which is why I just committed, but this PR is done, except I think we are waiting on you to add the API point: #899 !

@mikemhenry
Copy link
Contributor

I just sat down to work on adding that and saw that I needed a merge 😆

Copy link
Contributor

@ijpulidos ijpulidos left a comment

Choose a reason for hiding this comment

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

Looks good, great work!! Just a few changes that should be very easy to make.

perses/tests/test_relative_point_mutation_setup.py Outdated Show resolved Hide resolved
perses/tests/test_topology_proposal.py Outdated Show resolved Hide resolved
perses/tests/test_topology_proposal.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ijpulidos ijpulidos 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!

We just have to keep in mind the cost of having the LongRangeCorrection setting.

@ijpulidos ijpulidos merged commit 732dd87 into main Apr 13, 2022
@ijpulidos ijpulidos deleted the rest-over-protocol branch April 13, 2022 19:04
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.

None yet

5 participants