Reduce ridge filters memory footprints #6509
Conversation
…sult and taking max between iterations.
|
Hello @tkumpumaki! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-09-11 12:21:25 UTC |
|
@scikit-image/core since memory use is greatly improved with this PR, should we include a benchmark? |
There was a problem hiding this comment.
Excellent! Thank you @tkumpumaki 😉
I simply suggest a small code formatting, otherwise every think seems fine. @mkcor, not sure if a benchmark is necessary, but up to other @scikit-image/core members 🙂
I'd say yes. @tkumpumaki let us know if you want to do that yourself or we will happily do so for you if you are not that familiar with asv. |
I don't have a slightest clue how to test memory usage with the asv as python is not my main weapon. ;) |
no worries @tkumpumaki. @lagru, are you interested in adding the benchmark? If so that is great and we can wait for that, but if no one plans to work on it soon let's not hold up the PR over it. Certainly the memory usage will be lower with this change and I doubt there is a substantial change in runtime. There will be more calls to maximum, but each one does less work and the maximum operations are not the most expensive operations in this function in any case. Even if there were a small performance decrease, I would still vote to merge this for the clear memory efficiency gain! |
|
@grlee77 happy to. I'll do it in a minute. |
Ony my machine |
Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>
|
@tkumpumaki, I pushed to your branch. To avoid conflicts, I recommend pulling these changes if and before you add commits yourself again: |
|
The stochastic RANSAC failure on CI is unrelated. |
|
Thanks for the quick update, @lagru . While we are adding the benchmarks, can you also add |
|
It seems that the label |
My machine, with CPU benchmarks added |
|
Hi @tkumpumaki, I merged a larger refactor of ridge filters by @anntzer via #6446 today. It has created conflicts here, but more importantly it had also refactored these same sections and removed the upfront memory allocations! Given that, it may be that no code changes are necessary in My apologies for not realizing the full overlap between these two and pointing it out earlier. Let us know if you need help resolving conflicts here. |
|
That PR removed the large upfront memory allocations but still first computes the filters for all sigmas before taking the max (so there's a large memory use just before the max is taken); it may still be useful to replace that by computing the pairwise maxes after each sigma computation. |
…-memory-use # Conflicts: # skimage/filters/ridges.py
|
I merged main branch back to this branch and replaced ridges.py with the new one from the main. Then added memory fix again. |
|
I'm kind of confused by the commit hash that is used by the benchmark workflow which is d3e5df6. It looks like a merge commit but is not listed on this PR... |
|
But it shows an decreased memory use locally: Click to expand |
Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>
|
Thanks everyone! |
Right... It looks as if CI, instead of running directly for the latest commit (i.e., 4a79238), would merge this very commit into an ad hoc branch before running it. |
see: scikit-image/scikit-image#6149 scikit-image/scikit-image#6440 scikit-image/scikit-image#6446 scikit-image/scikit-image#6509 These fix various bugs, simplify the implementaiton and reduce the memory footprint
…y footprint) (#423) related to #419 A large overhaul of the ridge filters, addressing inaccuracies and errors has been implemented for scikit-image 0.20. This PR ports the same changes to these functions to cuCIM. upstream PRs: - scikit-image/scikit-image#6149 - scikit-image/scikit-image#6440 - scikit-image/scikit-image#6446 - scikit-image/scikit-image#6509 These fix various bugs, simplify the implementation and reduce the memory footprint Authors: - Gregory Lee (https://github.com/grlee77) Approvers: - Gigon Bae (https://github.com/gigony) URL: #423
Description
Reduce ridge filters memory footprints by not storing intermediate results for final max computation. Instead after each sigma take max between last and current sigma result.
Closes #6507
For reviewers
later.
__init__.py.doc/release/release_dev.rst.example, to backport to v0.19.x after merging, add the following in a PR
comment:
@meeseeksdev backport to v0.19.xrun-benchmarklabel. To rerun, the labelcan be removed and then added again. The benchmark output can be checked in
the "Actions" tab.