8000 BUG: random_walker: fix import path for umfpack by grlee77 · Pull Request #5090 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

BUG: random_walker: fix import path for umfpack #5090

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
Merged
merged 1 commit into from
Dec 5, 2020
Merged

BUG: random_walker: fix import path for umfpack #5090

merged 1 commit into from
Dec 5, 2020

Conversation

grlee77
Copy link
Contributor
@grlee77 grlee77 commented Nov 30, 2020

Description

random_walker seems to have a code path to allow use of UMFPACK, but it does not seem to get used even if scikit-umfpack is installed. The issue seems to be that the umfpack module is not visible via scipy.sparse.linalg.dsolve, but only within the scipy.sparse.linalg.dsolve.linsolve submodule. As far as I could tell, it has been this way in SciPy for many years. I verified locally that the tests still pass when scikit-umfpack is installed and added a new import test, although I don't think we ever run with scikit-umfpack on CI currently.

I thought about adding scikit-umfpack to requirements/optional.txt, but it does not have wheels on PyPI for recent Python or for Windows, so I have left it out for now.

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@emmanuelle
Copy link
Member

Thanks @grlee77. I even wonder if we still need this warning. I was the one to play initially with the random walker algorithm, and to notice that things could be faster with umfpack. But I guess most people don't have it installed, and with the default mode using the Jacobi preconditioner maybe we could get rid of this warning altogether?

@jni jni merged commit ac4d4bd into scikit-image:master Dec 5, 2020
@jni
Copy link
Member
jni commented Dec 5, 2020

I agree with your comments @emmanuelle but something for future generations to worry about. =P

@grlee77 grlee77 mentioned this pull request Dec 7, 2020
Closed
@grlee77 grlee77 deleted the random_walker_umfpack_import_fix branch July 8, 2021 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmanuelle emmanuelle emmanuelle approved these changes

@jni jni jni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@grlee77 @emmanuelle @jni
0