Fix cutoff and lifting parameters in RESTCapableHybridTopologyFactory

[zhang-ivy]

Description

This PR addresses the following problem:

The RESTCapableHybridTopologyFactory is not performing lifting to the 4th dimension correctly in vacuum. We are maximally lifting to 100 nm, which is much too large. The maximal lifting distance is currently defined by w_scale * r_cutoff. For vacuum, we set r_cutoff to be 100 nm, which is the distance we want to use when calling CustomNonbondedForce.setCutoffDistance(), but we do not want the maximal lifting distance to be that large, therefore, we need to remove the dependence of the maximal lifting distance on r_cutoff.

In fact, we actually don't need the maximal lifting distance to depend on 2 parameters, we can use just 1 parameter, which I'll call w_lifting, that defines the maximal lifting distance in nanometers.

This PR does the following:

• Renames r_cutoff to cutoff. cutoff is copied from the real system and if the nonbonded method is vacuum, cutoff is set to 100 nm. cutoff is only used when setting the cutoff distance in the CustomNonbondedForces via setCutoffDistance and not in the custom expression when defining lifting expressions.
• When defining the lifting expressions, use a single user-controlled parameter (w_lifting)

Motivation and context

How has this been tested?

Change log

Bug fix in the `RESTCapableHybridTopologyFactory` lifting expression -- fixed by separating the cutoff distance from the lifting distance.

[codecov]

Codecov Report

Merging #1046 (3c866c8) into main (28885ab) will not change coverage.
The diff coverage is 80.95%.

[ijpulidos]

The change of the parameter name from w_scale to w_lifting breaks API compatibility. That is, people using perses 0.10.0 will have problem getting their code to work with subsequent versions, if we decide to change this. I know we probably don't have many users of this new HTF yet. Is there a special reason why we would like to change the name? Just curious if it is worth to break the compatibility.

Also, if we decide to change this, then I think there are also some other things we have to review in setup_relative_calculation.py and the parent PointMutationExecutor, which are using w_scale.

[ijpulidos]

I guess the bigger concern I see, is that it could also just silently fail, if the user decide to use w_scale but the actual name is changed to w_lifting, as far as I can see.

[zhang-ivy]

The change of the parameter name from w_scale to w_lifting breaks API compatibility. That is, people using perses 0.10.0 will have problem getting their code to work with subsequent versions, if we decide to change this. I know we probably don't have many users of this new HTF yet. Is there a special reason why we would like to change the name? Just curious if it is worth to break the compatibility.

I've updated the description above to be more clear -- hopefully its now more obvious why this PR is necessary. More specifically, the name change is needed because w_scale no longer reflects what the argument represents. It is now a distance, not a scaling factor.

The RESTCapableHybridTopologyFactory is still pretty experimental and under development. I think the first priority is to make sure that code works properly, and the second priority is backwards compatibility. Once we reach a more stable, working version, I think we can prioritize backwards compatibility, but I don't think we are that point now.

Also, in the last release (0.10.0), we introduced at least one PR that break backwards compatibility: #1024
But I think this is ok, because we are still iterating on the best version of the API and we aren't advertising the API as stable yet.

Also, if we decide to change this, then I think there are also some other things we have to review in setup_relative_calculation.py and the parent PointMutationExecutor, which are using w_scale.

I can add these changes today.

[dominicrufa]

this looks fine. it seems there's quite a bit of code duplication though. might want to think about revising this at some point.

to clarify, this is a better handler for vacuum lifting, correct? otherwise you would have been lifing to ~100nm, which is probably problematic w.r.t. convergence?

[dominicrufa]

you found that 0.3nm is a good lifting value?

[zhang-ivy]

Yes, from preliminary experiments I found that 0.3 was good. I still need to run the w_lifting optimization experiments for the paper, but hopefully the results will be similar to the preliminary experiments.

[zhang-ivy]

to clarify, this is a better handler for vacuum lifting, correct? otherwise you would have been lifing to ~100nm, which is probably problematic w.r.t. convergence?

@dominicrufa : yes, see the description of the PR for more details on this.

[ijpulidos]

I think we can afford the API-breaking changes. I like that we are using explicit units in the parameters. Looks good!

[zhang-ivy]

The rest factory is still pretty experimental, so I don’t think it’s essential to ensure backwards compatibility at this point. I changed the name because originally we were using 2 parameters (w_scale and r_cutoff) to control one distance — this is 1) wrong (the lifting distance should not depend on the cutoff distance) and 2) unnecessary. To solve problem 1, I removed the dependence on r_cutoff. At this point, I could introduce a new user-defined parameter called lifting_cutoff, but then problem 2 arises, where now we are telling the user to specify w_scale and lifting_cutoff to compute the distance, which is w_scale* lifting_cutoff. It would be simpler to just tell the user to specify the distance directly, which I’ve named w_lifting. Not tied to this name, but I do think all the changes in this PR are necessary. On Jun 15, 2022, at 17:18, Iván Pulido ***@***.***> wrote:  I guess the bigger problem is that it could also just silently fail, if the user decide to use w_scale but the actual name is changed to w_lifting, as far as I can see. — Reply to this email directly, view it on GitHub<https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fchoderalab%2Fperses%2Fpull%2F1046%23issuecomment-1156956077&data=05%7C01%7Civy.zhang%40wustl.edu%7C900fc2d78e484263d52108da4f148b22%7C4ccca3b571cd4e6d974b4d9beb96c6d6%7C0%7C0%7C637909246891844830%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=czCSHzeeBgzVm6paRWZtFYsSwMeCbfqIs6E%2FhofR6IQ%3D&reserved=0>, or unsubscribe<https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAIPGJCRCCT476GWBQCMFZCTVPJCA3ANCNFSM5Y4P63GA&data=05%7C01%7Civy.zhang%40wustl.edu%7C900fc2d78e484263d52108da4f148b22%7C4ccca3b571cd4e6d974b4d9beb96c6d6%7C0%7C0%7C637909246891844830%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=PEqHLnJe6O%2BWGYNn8XGZah%2FWwQCsVXGveLuB8EBKiIA%3D&reserved=0>. You are receiving this because you authored the thread.Message ID: ***@***.***>