Fixed #1151 Forward query_idx from aamp_match to core.mass_absolute#1152
Merged
Merged
Conversation
…absolute core._find_matches seeds candidate_idx = query_idx and then breaks when D[candidate_idx] > atol + max_distance, so it relies on D[query_idx] == 0. core.mass_absolute establishes that, but aamp_match never passed query_idx to it, unlike motifs.match which passes it to core.mass. query_idx was added to core.mass and core.mass_absolute in stumpy-dev#860; stumpy-dev#856 wired it into core.mass from motifs.match the next day, but aamp_motifs.py was not updated. core.mass_absolute's query_idx branches have been unreachable since, which is why they carry `# pragma: no cover`. They are now covered, so the pragmas are removed. For a correct self-join the fix is a no-op: cdist returns exactly 0.0 for the identical row, so D[query_idx] was already the minimum. What it restores is the "self-match is returned first" guarantee and the "Subsequences `Q` and `T[query_idx:query_idx+m]` are different" warning when query_idx does not describe Q -- neither of which could happen on the non-normalized path.
|
Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1152 |
Contributor
|
@cagataykenter Thank you for the thorough explanation and for discovering this issue. It is greatly appreciated! Please allow me some time to review |
Contributor
|
@cagataykenter Thank you again for this excellent contribution! I am curious how you discovered this issue and what you use STUMPY for? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1151.
aamp_matchacceptsquery_idxand forwards it tocore._find_matches, but never forwards it tocore.mass_absolute. The normalized twin,motifs.match, forwards it to bothcore.massandcore._find_matches.core._find_matchesseedscandidate_idx = query_idxand then breaks whenD[candidate_idx] > atol + max_distance, so it depends onD[query_idx] == 0— which is whatcore.mass_absolute'squery_idxparameter exists to establish.query_idxwas added tocore.massandcore.mass_absolutein #860; #856 wired it intocore.massfrommotifs.matchthe next day, butaamp_motifs.pywas not updated.core.mass_absolute'squery_idxbranches have been unreachable ever since — which is why they carry# pragma: no cover. They are now covered, so the pragmas are removed.Changes
stumpy/aamp_motifs.py— forwardquery_idxtocore.mass_absolute.stumpy/core.py— drop the two# pragma: no covermarkers onmass_absolute's now-reachablequery_idxbranches.tests/test_aamp_motifs.py— addtest_aamp_match_query_idx. There was previously noquery_idxtest in eithertests/test_motifs.pyortests/test_aamp_motifs.py.Behaviour
For a correct self-join this is a no-op:
_mass_absoluteusesscipy.spatial.distance.cdist, which returns exactly0.0for the identical row, soD[query_idx]was already the minimum and the self-match was already returned first.What it restores is the behaviour when
query_idxdoes not describeQ— the casemass_absolute's warning exists to catch:matchaamp_matchbeforeaamp_matchafter(1, 2)(0,)(1, 2)Verification
main(np.array_equal→True), single- and multi-dimensional.aamp_motifs(the internal caller, which always passesquery_idx=candidate_idxwith the real subsequence): identicalmotif_distancesandmotif_indicesacross 30 seeds.tests/test_aamp_motifs.py,tests/test_motifs.py,tests/test_precision.py: all pass underNUMBA_DISABLE_JIT=1.core.py:1375,:1376,:1410,:1411with no partial branches, socoverage report --fail-under=100still passes with the pragmas gone.black --check,isort --profile black --check-only,flake8: clean.