Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRefactor and fix peak_local_max #4760
Conversation
Co-authored-by: Marianne Corvellec <[email protected]>
…into Fix_peack_local_max
|
Thank you @mkcor for your review ;) |
rfezzani
changed the title
…into Fix_peack_local_max
|
Can we plan this PR for 0.18 or is the schedule too short? |
| tree = spatial.cKDTree(coord) | ||
|
|
||
| indices = tree.query_ball_point(coord, r=min_distance, p=p_norm) | ||
| rejected_peaks_indices = set() | ||
| for idx, candidates in enumerate(indices): | ||
| if idx not in rejected_peaks_indices: | ||
| candidates.remove(idx) | ||
| rejected_peaks_indices.update(candidates) | ||
|
|
||
| # Remove the peaks that are too close to each other | ||
| coord = np.delete(coord, tuple(rejected_peaks_indices), axis=0) |
lagru
Sep 29, 2020
Member
Several suggestions here:
- We can skip this step if
min_distance is 1, correct?
- Also, it's probably best to update the description of that parameter in the docstring similar to #4218? I think the part with "2 * min_distance + 1" could be clearer.
- As already suggested in #4218 it's probably best to factor this out? Maybe even to Cython to get rid of the slow loop (doesn't have to be this PR)?
Several suggestions here:
- We can skip this step if
min_distanceis 1, correct? - Also, it's probably best to update the description of that parameter in the docstring similar to #4218? I think the part with "2 * min_distance + 1" could be clearer.
- As already suggested in #4218 it's probably best to factor this out? Maybe even to Cython to get rid of the slow loop (doesn't have to be this PR)?
rfezzani
Oct 2, 2020
Author
Member
Thank you @lagru for your review, and sorry for my late answer, I have been too much busy these last few days 🙏 .
- We can skip this step if min_distance is 1, correct?
No, we also want to remove the adjacent peaks (I may be didn't get your point here 😕 ).
- Also, it's probably best to update the description of that parameter in the docstring similar to #4218? I think the part with "2 * min_distance + 1" could be clearer.
👍
- As already suggested in #4218 it's probably best to factor this out? Maybe even to Cython to get rid of the slow loop (doesn't have to be this PR)?
Why not, I remember that we already mentioned this ;)
Thank you @lagru for your review, and sorry for my late answer, I have been too much busy these last few days
- We can skip this step if min_distance is 1, correct?
No, we also want to remove the adjacent peaks (I may be didn't get your point here
- Also, it's probably best to update the description of that parameter in the docstring similar to #4218? I think the part with "2 * min_distance + 1" could be clearer.
- As already suggested in #4218 it's probably best to factor this out? Maybe even to Cython to get rid of the slow loop (doesn't have to be this PR)?
Why not, I remember that we already mentioned this ;)
rfezzani
Oct 7, 2020
Author
Member
@lagru, I factored out this part as you suggested in the skimage._shared.coord.ensure_spacing function, I hope you agree with the function name and its API.
@lagru, I factored out this part as you suggested in the skimage._shared.coord.ensure_spacing function, I hope you agree with the function name and its API.
lagru
Oct 8, 2020
Member
Thank you. The name seems fine to me. Did you intentionally not replace the appropriate code snippet in corner_peaks with ensure_spacing?
Thank you. The name seems fine to me. Did you intentionally not replace the appropriate code snippet in corner_peaks with ensure_spacing?
rfezzani
Oct 8, 2020
Author
Member
With this Fix of peak_local_max, corner_peaks becomes obsolete and can directly be replaced by the former.
With this Fix of peak_local_max, corner_peaks becomes obsolete and can directly be replaced by the former.
Description
In my attempt to fix #2592, I made a review of the code and found many issues (see #4756, #4749, #4752 and #3990 (comment)). It appeared to me that a global refactoring of
peak_local_maxis necessary.Multiple changes are applied in this PR:
min_distanceis enforced in the same way as in #4218 forcorner_peak(see here)_get_thresholdand_get_excluded_border_widthare introduced for readabilitylabelsrenumbering is avoidedIf accepted, this PR allows to deprecate
corner_peakthat becomes a duplicate ofpeak_local_max.Fixes #2592. Fixes #4756. Fixes #4756. Closes #3990.
Checklist
./doc/examples(new features only)./benchmarks, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py.doc/release/release_dev.rst.