Skip to content

Add --multiple-threshold for returning all peaks above a score - #67

Open
tandav wants to merge 2 commits into
bbc:masterfrom
tandav:multiple-matches
Open

tandav wants to merge 2 commits into
bbc:masterfrom
tandav:multiple-matches

Conversation

@tandav

@tandav tandav commented May 28, 2026

Copy link
Copy Markdown

This PR solves #61

tandav and others added 2 commits May 28, 2026 09:34
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@elvum

elvum commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Unsurprisingly given that I (as my alter-ego @stephenjolly) expressed support for this in #61, I think this would be a useful feature to add to the tool. At least for me, it fits the "minimal, but widely useful" criterion that I have in the past espoused. However, I would like to suggest a few changes to this PR that should be made before it is ready to merge:

  1. The new --multiple-threshold mode ignores a zero-offset match. For example, search for one of the test files within itself. Without the new flag, the old algorithm correctly reports an offset at 0.0. With the new flag, it reports "Found 0 peak(s)." Specifically, this is because the new code runs SciPy's find_peaks function on the wrapped array, which makes an incorrect assumption that neighbouring array elements are neighbouring offsets. This isn't true for the endpoints, or for the join in the middle. The solution is to use the existing reorder_correlations function that is currently in cli.py before searching the array, ideally be moving that function out of the cli tool and into the library.
  2. The "--start" parameter is not related to the problem this PR fixes - I recommend removing it from this PR and leaving it for Add --start and per-file trim/start flags for narrowing the search to a specific time window #68 to address.
  3. Now that Fix the earliest negative correlation offset #70 has merged, this PR should be rebased on it, to avoid re-introducing that bug.

I think it would also be worth noting in the docs / --help output that the new flag changes the json output format from an object to an array.

This branch has not been deployed

No deployments
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

Successfully merging this pull request may close these issues.

2 participants