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.

Sign up for GitHub

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

Jump to bottom

Documentation for reconstructing square distance matrix from self_distance_array is incorrect. #4731

Open
hmacdope opened this issue Oct 13, 2024 · 4 comments · May be fixed by #4837
Open

Documentation for reconstructing square distance matrix from self_distance_array is incorrect. #4731

hmacdope opened this issue Oct 13, 2024 · 4 comments · May be fixed by #4837
Labels
Component-Docs defect

Comments

@hmacdope
Copy link
Member

The documentation for self_distance_array has the following as the suggested way to reconstruct a full NxN distance matrix from the flattened upper triangular (-diagonal) N*(N-2) // 2 result.

for i in range(n):
    for j in range(i + 1, n):
        k += 1
        dist[i, j] = d[k]

This is incorrect as the increment must come after the assignment, otherwise you attempt to access N+1 the element of the array on final iteration.

The below is correct.

for i in range(n):
    for j in range(i + 1, n):
        dist[i, j] = d[k]
        k += 1
@orbeckst
Copy link
Member

Shouldn't we also add the transpose right away

dist[i, j] = dist[j, i] = d[k]

if we want the full NxN matrix?

@hmacdope
Copy link
Member Author

Yea that as well

@tylerjereddy
Copy link
Member

Any reason not to suggest using squareform for this?

@orbeckst
Copy link
Member

No good reason — I didn't know about squareform.

There's the pedagogical advantage of showing exactly what the relationship between input and output is but the scipy docs also contain it.

One would just need to check that our output of self_distance_array is actually compatible with the scipy convention.

@tanishy7777 tanishy7777 linked a pull request Dec 15, 2024 that will close this issue
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Component-Docs defect
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #4731 tanishy7777/mdanalysis
3 participants
@orbeckst @tylerjereddy @hmacdope