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

IntegratorSettings don't all apply to the MD Protocol #774

Open
IAlibay opened this issue Mar 20, 2024 · 2 comments
Open

IntegratorSettings don't all apply to the MD Protocol #774

IAlibay opened this issue Mar 20, 2024 · 2 comments

Comments

@IAlibay
Copy link
Contributor

IAlibay commented Mar 20, 2024

Looking at the IntegratorSettings, reassign_velocities, n_restart_attempts, and constraint_tolerance, don't get used in the PlainMDProtocol.

This is because the PlainMDProtocol uses a LangevinMiddleIntegrator, whilst the RFE and AFE Protocols use LangevinDynamicsMove.

The difference in use of integrators is expected, but the different classes should really be used to avoid user confusion.

@IAlibay IAlibay added this to the 1.0.0 milestone Mar 20, 2024
@mikemhenry
Copy link
Contributor

mikemhenry commented Mar 25, 2024

perhaps we could add warnings if they are set in the protocol/"this setting isn't used by the protocol and will be removed in the future"

@richardjgowers richardjgowers removed this from the 1.0.0 milestone Mar 26, 2024
@richardjgowers
Copy link
Contributor

if I've understood this right, there's fields that you can set that do nothing in an MD protocol; which is confusing but not harmful.

I think the fix for this can be to deprecate the IntegratorSettings usage in MDProtocol in favour of the correct fields, with appropriate warnings etc, but it can be done in 1.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants