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

Update taper max_percentage #2492

Closed
wants to merge 1 commit into from
Closed

Update taper max_percentage #2492

wants to merge 1 commit into from

Conversation

zackspica
Copy link

if not set to None, will not respond to other param like max_length.

What does this PR do?

Please fill in

Why was it initiated? Any relevant Issues?

Please fill in

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+ DOCS"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

if not set to None, will not respond to other param like max_length.
@megies
Copy link
Member

megies commented Oct 29, 2019

I do not understand the motivation for this change, can you elaborate please?

@zackspica
Copy link
Author

zackspica commented Oct 29, 2019 via email

@megies
Copy link
Member

megies commented Oct 29, 2019

Yes, because max_percentage is required. This is partly due to historic reasons, and requiring the percentage option over an absolute length option as mandatory is a matter of taste, I guess. With this proposed change, using .taper() with default arguments and no customization would basically do nothing to the data. I don't think that's a better solution compared to forcing input by users.

Also this is a non-problem, just call it with taper(max_percentage=None, max_length=...)

@megies megies closed this Oct 29, 2019
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