-
Notifications
You must be signed in to change notification settings - Fork 61
feat!: mark the total forces invalid for startup steps #899
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
base: remove_unused_member
Are you sure you want to change the base?
Conversation
| void onBuffersUpdated() override; | ||
| void calculate() override; | ||
| void setStep(int64_t step) override; | ||
| void setStep(int64_t step, int startup, int doMigration) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we try to maintain backward compatibility in the API, but since cudaGM is an experimental feature, I suppose it's not necessary here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree that it's fine here, but it's also a legitimate point to bring it up, since as of right now this code is interfaced to NAMD as a dynamically loaded plugin
cc92c96 to
ddb8583
Compare
giacomofiorin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean extension of #887 to the CUDAGM interface, but I agree it's good to keep this separate since it depends on a MR in the NAMD repo that changes the CUDAGlobalMaster API.
| void onBuffersUpdated() override; | ||
| void calculate() override; | ||
| void setStep(int64_t step) override; | ||
| void setStep(int64_t step, int startup, int doMigration) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree that it's fine here, but it's also a legitimate point to bring it up, since as of right now this code is interfaced to NAMD as a dynamically loaded plugin
src/colvar_gpu_calc.cpp
Outdated
| if (colvar_module->step_relative() > 0) { | ||
| checkColvarsError(cvc_calc_total_force(colvars, colvar_module, false)); | ||
| } | ||
| checkColvarsError(cvc_calc_total_force(colvars, colvar_module, false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with removing the additional if, since the function checks internally for the "valid" flag, but maybe add a 1-line comment here to mention that?
55a4ffe to
1e3d569
Compare
ddb8583 to
cadcfa0
Compare
cadcfa0 to
836fae4
Compare
This PR depends on the NAMD merge request 492 (https://gitlab.com/tcbgUIUC/namd/-/merge_requests/492).