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

Fix gradients and metric functions of distanceDir #724

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

giacomofiorin
Copy link
Member

Fixes #714

Types indicating differences or derivatives of unit vectors and unit
quaternions are needed to handle values with the same dimensionality as the
original types, but no constraints.

The two enum also need to be handled explicitly in a switch statement to
compile without warnings.  However, it is better if the corresponding code
raises an error, instead of computing something that has no consistent
definition.
@giacomofiorin giacomofiorin added the bugfix To be used only in PRs label Oct 10, 2024
@HanatoK
Copy link
Member

HanatoK commented Oct 11, 2024

If this one is merged before #713, I will need to update the test file introduced in #713.

@giacomofiorin
Copy link
Member Author

If this one is merged before #713, I will need to update the test file introduced in #713.

Entirely up to you. I already looked at the code in #713 and it looks pretty clear, so I think we can merge either way. Let me write a review there.

@giacomofiorin giacomofiorin marked this pull request as ready for review October 11, 2024 01:22
Copy link
Member

@HanatoK HanatoK 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 to me!

src/colvarvalue.cpp Show resolved Hide resolved
{
return (x1.rvector_value - x2.rvector_value).norm2();
return x1.dist2(x2);
Copy link
Member

Choose a reason for hiding this comment

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

So now the square of distance of two distanceDir is calculated as the angle between them. Should we clarify that in the documentation?

After merging this PR and #713 , I may still need to verify if polarPhi and distanceDir with a custom function could yield the same trajectories.

Copy link
Member Author

@giacomofiorin giacomofiorin Oct 11, 2024

Choose a reason for hiding this comment

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

So now the square of distance of two distanceDir is calculated as the angle between them. Should we clarify that in the documentation?

Yes. I meant to do that, but forgot. I would say, though, that defining the distance as such is the same as taking the standard Euclidean distance (L^2 norm) but making it comply with the unit-vector constraint.

After merging this PR and #713 , I may still need to verify if polarPhi and distanceDir with a custom function could yield the same trajectories.

Yeah, that makes sense. (EDIT: the choice of metrics in colvarvalue should not matter there, only that the gradients of colvar::distance_dir are correct.)

@giacomofiorin giacomofiorin merged commit eaf0e59 into master Oct 11, 2024
14 checks passed
@giacomofiorin
Copy link
Member Author

@HanatoK I merged this for now before #713, where Jérôme might need to take a look. Will port into the namd-3.0 branch as well.

@giacomofiorin giacomofiorin deleted the fix-distanceDir branch October 11, 2024 19:25
HanatoK added a commit to HanatoK/colvars that referenced this pull request Oct 11, 2024
giacomofiorin pushed a commit to HanatoK/colvars that referenced this pull request Oct 11, 2024
jhenin pushed a commit that referenced this pull request Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix To be used only in PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

distance_dir::apply_force seems incorrect
2 participants