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

_build_skeleton_path_graph() incorrectly handles degree-0 nodes in calculation of n_points #182

Closed
ssavary opened this issue Dec 1, 2022 · 2 comments

Comments

@ssavary
Copy link

ssavary commented Dec 1, 2022

skan/src/skan/csr.py

Lines 395 to 400 in 1f71a5b

endpoints = (degrees != 2)
endpoint_degrees = degrees[endpoints]
num_paths = np.sum(endpoint_degrees)
path_indptr = np.zeros(num_paths + buffer_size_offset, dtype=int)
# the number of points that we need to save to store all skeleton
# paths is equal to the number of pixels plus the sum of endpoint

skan/src/skan/csr.py

Lines 406 to 409 in 1f71a5b

n_points = (
graph.indices.size + np.sum(endpoint_degrees - 1)
+ buffer_size_offset
)

Degree 0 nodes are mistakenly removed from the set. The correction calculation should be:

n_points = (
        graph.indices.size + np.sum(np.max(endpoint_degrees - 1, 0))
        + _buffer_size_offset
        )

With very degenerate cases, I get n_points = 0 and memory dumps occur (on linux). The real issue is the n_points computation, not the memory dump, which is only a consequence of bad n_points).

Here is a matrix that triggers the bug (for some reason the true/except block also triggers the core dump immediately, otherwise it takes a few calls to car.Skeleton):

import numpy as np
from skan import csr

X = np.array([1, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 1]).astype(bool)
try:
     S = csr.Skeleton(X)
except Exception as E:
     print(E)

@ssavary ssavary changed the title n_points seems to incorrectly handle degree-0 nodes in _build_skeleton_path_graph() _build_skeleton_path_graph() incorrectly handles degree-0 nodes in calculating n_points Dec 1, 2022
@ssavary ssavary changed the title _build_skeleton_path_graph() incorrectly handles degree-0 nodes in calculating n_points _build_skeleton_path_graph() incorrectly handles degree-0 nodes in calculation of n_points Dec 1, 2022
@jni
Copy link
Owner

jni commented Dec 2, 2022

Yikes, good catch. Thank you for the thorough report!

the correction calculation should be

Do you mean np.maximum rather than np.max?

In [1]: np.max([0, 9, -2, 0, 9], 0)
Out[1]: 9

In [2]: np.maximum([0, 9, -2, 0, 9], 0)
Out[2]: array([0, 9, 0, 0, 9])

Anyway, would you be able to/like guidance creating a pull request with a fix and a test? 🙏

@ssavary
Copy link
Author

ssavary commented Dec 2, 2022

Do you mean np.maximum rather than np.max?

Yes you are right! np.maximum is the right one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants