Add support for MDA minimize_vectors#188
Open
PardhavMaradani wants to merge 6 commits intoMDAnalysis:mainfrom
Open
Add support for MDA minimize_vectors#188PardhavMaradani wants to merge 6 commits intoMDAnalysis:mainfrom
PardhavMaradani wants to merge 6 commits intoMDAnalysis:mainfrom
Conversation
Contributor
Author
|
Hi, I am unable to figure out the build error below. Could you please help me and let me know if there is something I am missing? I was able to build this locally on my ubuntu environment, which is how I was able to run the performance tests with a working version in the referenced MDA PR #5235. Thanks |
Contributor
Author
I noticed that the 'Read the Docs' build has been failing for quite some time. The rest of the tests didn't seem to run and I see the following notice for that workflow: 
Could someone with access please re-enable this for tests to run again. Thanks |
Contributor
Author
|
Here are the benchmark results comparing against mda version (serial): Show a full benchmark results runHere are some comparative results using MDAnalysis's own |
Member
|
I re-enabled the workflow. Try pushing something to get the tests to run. |
Member
|
@hmacdope could you review, please? |
Contributor
Author
I pushed an empty commit. It seems like the workflow requires an approval to run. Thanks |
Contributor
Author
|
About the test failures:
|
Contributor
Author
|
The macos build failures show the following errors: These seem to be fixed in the latest (v1.9.5) of google benchmark (through their PR google/benchmark#2108). Our submodule was pointing to an older commit |
Member
|
@PardhavMaradani I changed the repo settings for actions. Hopefully, you won't need approval for re-running CI. If it's still necessary please let me know. |
Contributor
Author
|
Updating google benchmark to v1.9.5 fixed the macos build issues. The same commit ( Capturing the relevant sections of the logs here so this is preserved: DetailsAdding a |
Member
|
Someone like @hmacdope or @richardjgowers would be the most appropriate people to review here. I just don't know how busy they are. |
Closed
6 tasks
orbeckst
requested changes
Apr 25, 2026
Member
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
My primary concern is about the source of the code in test/compare/calc_distances.hpp. We first need to have some clarity on what to do there.
| Copyright (c) 2006-2017 The MDAnalysis Development Team and contributors | ||
| (see the file AUTHORS for the full list of names) | ||
|
|
||
| Released under the Lesser GNU Public Licence, v2.1 or any higher version |
Member
There was a problem hiding this comment.
@PardhavMaradani is the code below based on https://github.com/MDAnalysis/mdanalysis/blob/develop/package/MDAnalysis/lib/include/calc_distances.h ? If so then you cannot include it because distopia is MIT.
Member
There was a problem hiding this comment.
I assume it's actually based on compare/calc_distances.h.
This is still problematic but I get where you're coming from.
Contributor
Author
There was a problem hiding this comment.
Apologies if this introduced any licensing issues. I am yet to understand most of the licensing details. Here is more info about this file (calc_distances.hpp):
- This is the exact same file used in the corresponding MDA PR (Add backends support for minimize_vectors mdanalysis#5235)
- The code in this file is a direct translation of
cythoncode from MDAc_distances.pyxto c++ with templates and openmp directives - The old comments from the
cythoncode do indicate that they are based offcalc_distances.hwith minor changes - This file is used only in the c++ tests to compare
distopiacomputations against the corresponding MDA versions
I will wait for your guidance on how proceed further on this. Thanks
| V &vy, | ||
| V &vz) const { | ||
| ShiftIntoPrimaryUnitCell(vx, vy, vz); | ||
| MinimiseVectors(vx, vy, vz); |
Member
There was a problem hiding this comment.
British spelling? And that works with MimizeVectors (US English) below??
Member
There was a problem hiding this comment.
Was MinimizeVectorsInPrimaryUnitCell ever tested?
Contributor
Author
There was a problem hiding this comment.
Yes, this method is being tested - both from c++ tests (ctest), python tests (pytest) and is also invoked from the benchmarks. (Example for ortho case: distopia::MinimizeVectorsOrtho -> MinimizeVectorsOrthoHwy -> MinimizeVectors -> box.MinimizeVectorsInPrimaryUnitCell)
The name MinimiseVectors has been there from the beginning in this repo (see MDAnalysis/mdanalysis#3472 for MDA minimize_vectors implementation) but isn't quite what we need for this PR. I was initially unsure if I should go ahead and rename it to be consistent, but seems like what we have now is not great and confusing. I'll refactor these as follows:
- Change
MinimalVectorstoMinimalVector(singular) for better semantics (as suggested elsewhere in this PR too) - Instead of a new method like
MinimizeVectorsInPrimaryUnitCell, overloadMinimalVectorto handle the case of a single vector as well which shifts the vector into the primary cell befor calling below - Change
MinimizeVectorstoMinimizeVector, (singular) which is the core implementation for each box type - We will use
MinimalVectorsimilar to how it is used everywhere else in this code
The above should hopefully make this a bit more consistent. Please let me know if you think there is a better way to refactor this. Thanks
- Refactor MinimiseVectors to MinimizeVector (singulr) - Overload MinimalVector to support single vector - Refactor _minimize_vectors_* to _minimize_vector_* (singular) - MDA test code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds support for MDAnalysis
minimize_vectorsas described in MDA Issue #5234. MDA PR #5235 adds the backend support for this function in MDAnalysis. Thanks