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

remove masked arrays from salience #458

Merged
merged 1 commit into from Dec 3, 2016
Merged

Conversation

rabitt
Copy link
Contributor

@rabitt rabitt commented Dec 3, 2016

Removes masked arrays, performs peak masking after harmonic summation.


This change is Reviewable

@bmcfee
Copy link
Member

bmcfee commented Dec 3, 2016

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


librosa/core/harmonic.py, line 90 at r1 (raw file):

        S_peaks = scipy.signal.argrelmax(S, axis=0)
        peak_mask = np.zeros(S_harm.shape).astype(bool)
        peak_mask[:, S_peaks[0], S_peaks[1]] = True

peak mask should now be the shape of S_sal/S.

You could also compute this directly by saying peak_mask = util.localmax(S, axis=0)


librosa/core/harmonic.py, line 91 at r1 (raw file):

        peak_mask = np.zeros(S_harm.shape).astype(bool)
        peak_mask[:, S_peaks[0], S_peaks[1]] = True
        S_sal[peak_mask] = 0.0

Shouldn't this mask out everything except peak_mask?

Also, the fill value should be user-specified, not hard-coded to 0.


Comments from Reviewable

@bmcfee
Copy link
Member

bmcfee commented Dec 3, 2016

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


Comments from Reviewable

@bmcfee
Copy link
Member

bmcfee commented Dec 3, 2016

Reviewed 1 of 1 files at r3.
Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.


librosa/core/harmonic.py, line 44 at r3 (raw file):

    fill_value : float
        The value to fill non-peaks in the output representation. (default:
        np.nan) Only used if fitler_peaks = True.

fitler -> filter, = -> == (and put in scare-quotes)


Comments from Reviewable

@rabitt rabitt force-pushed the fix-salience branch 2 times, most recently from d2d3e8e to 72e0f21 Compare December 3, 2016 21:31
@bmcfee
Copy link
Member

bmcfee commented Dec 3, 2016

:lgtm:


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


Comments from Reviewable

@bmcfee bmcfee merged commit 2503af5 into librosa:master Dec 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants