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

Optimize the fit gradients #729

Merged
merged 9 commits into from
Nov 13, 2024

Conversation

HanatoK
Copy link
Member

@HanatoK HanatoK commented Oct 15, 2024

  • Save the unrotated positions. This can avoid rotating the positions again in calc_fit_forces_impl;
  • Determine dl, dq or ds by compile-time template options;
  • Only call calc_fit_gradients when atom groups have explicit gradients;
  • Simplify and inline cvm::quaternion::position_derivative_inner.

This PR should be only marked as ready after #713 is merged.

@HanatoK HanatoK force-pushed the optimize_fit_gradients branch 2 times, most recently from 09def4b to 2da467b Compare October 16, 2024 14:09
@giacomofiorin
Copy link
Member

@HanatoK We don't currently have a CI job with Intel, but that compiler uses restrict (as well as PGI pre-NVIDIA). See the following from the LAMMPS headers:
https://github.com/lammps/lammps/blob/59bbc5bcc1104bdb4fb45107cd65b5d4d76dbc00/src/lmptype.h#L342-L348

@HanatoK
Copy link
Member Author

HanatoK commented Oct 16, 2024

@HanatoK We don't currently have a CI job with Intel, but that compiler uses restrict (as well as PGI pre-NVIDIA). See the following from the LAMMPS headers: https://github.com/lammps/lammps/blob/59bbc5bcc1104bdb4fb45107cd65b5d4d76dbc00/src/lmptype.h#L342-L348

Thanks! I will try using the same way to apply __restrict.

@giacomofiorin
Copy link
Member

@HanatoK We don't currently have a CI job with Intel, but that compiler uses restrict (as well as PGI pre-NVIDIA). See the following from the LAMMPS headers: https://github.com/lammps/lammps/blob/59bbc5bcc1104bdb4fb45107cd65b5d4d76dbc00/src/lmptype.h#L342-L348

Thanks! I will try using the same way to apply __restrict.

Awesome! I added an Intel oneAPI CI job in the meantime, which is overkill for just this PR but definitely was a missing feature otherwise.

@HanatoK HanatoK force-pushed the optimize_fit_gradients branch from 643ed9e to 3de535c Compare October 16, 2024 18:54
@HanatoK
Copy link
Member Author

HanatoK commented Oct 17, 2024

I think it is better to document how the fit gradients are computed so I tried my best to improve the doxygen documentation in 2d353eb. I know the SI of the new Colvars paper includes the computation but that is too simplified and not quite informative.

@HanatoK HanatoK marked this pull request as ready for review October 17, 2024 22:11
@jhenin jhenin force-pushed the optimize_fit_gradients branch from 2d353eb to 8e0b9a2 Compare November 12, 2024 15:32
@jhenin jhenin self-requested a review November 12, 2024 15:32
Copy link
Member

@jhenin jhenin left a comment

Choose a reason for hiding this comment

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

Nice optimization I rebased and made tiny cosmetic changes, and I have one question. Apart from that, this is ready to merge IMO.

src/colvar_rotation_derivative.h Show resolved Hide resolved
@jhenin jhenin self-requested a review November 13, 2024 11:41
@giacomofiorin giacomofiorin self-requested a review November 13, 2024 15:53
Copy link
Member

@giacomofiorin giacomofiorin 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, apart from the speedup the code is also getting more readable.

@@ -1420,7 +1420,7 @@ FORMULA_TRANSPARENT = YES
# The default value is: NO.
# This tag requires that the tag GENERATE_HTML is set to YES.

USE_MATHJAX = NO
USE_MATHJAX = YES
Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks!

@giacomofiorin giacomofiorin merged commit 2a61c36 into Colvars:master Nov 13, 2024
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.

3 participants