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

Reassignment docstring typos #1043

Merged
merged 4 commits into from Dec 27, 2019
Merged

Reassignment docstring typos #1043

merged 4 commits into from Dec 27, 2019

Conversation

scjs
Copy link
Contributor

@scjs scjs commented Dec 17, 2019

Reference Issue

PR #1038

What does this implement/fix? Explain your changes.

Fixes typo: S_df to S_dh, and adds some missing periods. I also moved the reassigned_spectrogram argument recommendations to Notes in a separate commit.

Any other comments?

It looks like Notes is so far just used for notes about caching, so feel free to revert that commit if you prefer.

@bmcfee bmcfee added the documentation Issues relating to docstrings, examples, and documentation build label Dec 22, 2019
@bmcfee bmcfee added this to the 0.7.2 milestone Dec 22, 2019
Copy link
Member

@bmcfee bmcfee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- thanks for this! No problem with the notes section, your changes here seem appropriate.

Is there anything else to do here, or shall I merge?

@lostanlen lostanlen self-assigned this Dec 23, 2019
@scjs
Copy link
Contributor Author

scjs commented Dec 23, 2019

I ran pep8 and fixed some lint in the docstring, and made the S notation consistent. Go ahead and merge if it looks fine to you. Thanks!

@lostanlen
Copy link
Contributor

Thank you for the update, especially for the very eloquent Notes section. This makes me believe that perhaps we should change the pad_mode default?

Also, this problem seems like a big enough a deal to add a line about it in the list of input arguments (at the center), and refer to Notes for the whole spiel explaining the issue.

@bmcfee
Copy link
Member

bmcfee commented Dec 25, 2019

This makes me believe that perhaps we should change the pad_mode default?

I still disagree -- we've been over this before in the original PR, but it's more important to have API consistency on this so that frame alignment across methods is preserved by default.

@bmcfee
Copy link
Member

bmcfee commented Dec 27, 2019

:shipit: thanks again @scjs !

@bmcfee bmcfee merged commit 2c0f7fa into librosa:master Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues relating to docstrings, examples, and documentation build
Development

Successfully merging this pull request may close these issues.

None yet

3 participants