Skip to content
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.

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

Fixed potential KeyError if step_sizes_sigma contains numbers > 1 #401

Merged
merged 2 commits into from
Aug 9, 2016
Merged

Fixed potential KeyError if step_sizes_sigma contains numbers > 1 #401

merged 2 commits into from
Aug 9, 2016

Conversation

sonovice
Copy link
Contributor

@sonovice sonovice commented Aug 9, 2016

The backtracking could fail under certain conditions if the step sizes contain numbers that are larger than 1. This could produce a KeyError in condition of the while loop.

... Sorry for the autoformatting of my IDE.


This change is Reviewable

@bmcfee
Copy link
Member

bmcfee commented Aug 9, 2016

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


librosa/core/dtw.py, line 62 [r1] (raw file):

    # Calculate the radius in indices, rather than proportion
    radius = int(round(radius * np.min(x.shape)))

this should actually be np.round to preserve consistency between python 2 and python 3. Can you change that here?


librosa/core/dtw.py, line 331 [r1] (raw file):

    # Setting it to (0, 0) does not work for the subsequence dtw,
    # so we only ask to reach the first row of the matrix.
    while cur_idx[0] > 0:

I don't understand how this fixes the problem you describe in the PR; can you comment?


Comments from Reviewable

@sonovice
Copy link
Contributor Author

sonovice commented Aug 9, 2016

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


librosa/core/dtw.py, line 62 [r1] (raw file):

Previously, bmcfee (Brian McFee) wrote…

this should actually be np.round to preserve consistency between python 2 and python 3. Can you change that here?

Sure, will do that.

librosa/core/dtw.py, line 331 [r1] (raw file):

Previously, bmcfee (Brian McFee) wrote…

I don't understand how this fixes the problem you describe in the PR; can you comment?

`cur_idx` get's decreased by `step_sizes_sigma` and might get below 0 this way if the step size exceeds 1. Negative values however would not exit the while loop which I'm trying to avoid.

Comments from Reviewable

@bmcfee
Copy link
Member

bmcfee commented Aug 9, 2016

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bmcfee bmcfee added the bug Something doesn't work like it should label Aug 9, 2016
@bmcfee bmcfee added this to the 0.5 milestone Aug 9, 2016
@bmcfee
Copy link
Member

bmcfee commented Aug 9, 2016

Thanks! This looks good to me. Are you planning anything else here, or are we merge-ready?

@stefan-balke
Copy link
Member

Is there any strong reason to add all the whitespaces?

Otherwise LGTM.

@sonovice
Copy link
Contributor Author

sonovice commented Aug 9, 2016

The whitespaces are recommended in PEP8: https://www.python.org/dev/peps/pep-0008/#other-recommendations

This was just a quick bug fix, so no other commits are planed by now. (Though I would like to add multiscale DTW sometime soon, but that would be a totally different issue...)

@bmcfee bmcfee merged commit 71219b4 into librosa:master Aug 9, 2016
@sonovice sonovice deleted the fixDtw branch August 9, 2016 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something doesn't work like it should
Development

Successfully merging this pull request may close these issues.

None yet

3 participants