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

Clipping deformer & cosmetic fixes #80

Merged
merged 3 commits into from
Jul 20, 2020
Merged

Conversation

jatinkhilnani
Copy link
Contributor

@jatinkhilnani jatinkhilnani commented Jun 2, 2020

@bmcfee
Copy link
Owner

bmcfee commented Jun 16, 2020

It looks like this PR only includes the cosmetic fixes?

@jatinkhilnani
Copy link
Contributor Author

  • Included the deformer code clipping.py with tests
  • Have implemented the test parameterization both in the current and new syntax, that eliminates the warnings with pytest latest version (5.4.3)
  • Added comments for test_deformers
  • Open item: Per new syntax, added expectation (type of error) raised for xfail markers for Clipping method. The same is not working for LinearClipping & RandomClipping

@jatinkhilnani jatinkhilnani marked this pull request as ready for review June 18, 2020 03:48
@jatinkhilnani jatinkhilnani changed the title Clip deformer & cosmetic fixes Clipping deformer & cosmetic fixes Jun 18, 2020
muda/deformers/clipping.py Outdated Show resolved Hide resolved
tests/test_deformers.py Outdated Show resolved Hide resolved
Copy link
Owner

@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.

A couple of minor cosmetic things, but this all looks good otherwise. Thanks!

@jatinkhilnani
Copy link
Contributor Author

jatinkhilnani commented Jun 18, 2020

  • Have also segregated and commented the deprecated marker syntax for better readability. Would we want these changes to be done for older tests too?
  • Do I need to take care of the CI check failure, seems to be doing so for older Python versions. Also, please suggest if there are there any other documentation changes required for this before I move on to complete the other deformer.

@jatinkhilnani
Copy link
Contributor Author

Along with changes per review comments, included the ones mentioned in comment above (not for older tests). Modified config to include pytest-cov==2.9.0, since v2.10.0 required pytest>=4.6 creating a conflict and causing build failure.

This does not completely resolve the CI check issues due to llvmlite>=0.31.0dev0->numba>=0.43.0->librosa>=0.4, as both the former packages are now compatible only with Python>=3.6. I see that librosa CI checks are now only performed for compatible Python versions. I suppose that needs to be modified here as well, or is there a workaround?

Copy link
Contributor Author

@jatinkhilnani jatinkhilnani left a comment

Choose a reason for hiding this comment

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

Checked that the requested changes are applied. Please review and merge.

@jatinkhilnani jatinkhilnani requested a review from bmcfee June 22, 2020 22:01
@bmcfee bmcfee merged commit eeeaf6d into bmcfee:master Jul 20, 2020
@bmcfee
Copy link
Owner

bmcfee commented Jul 20, 2020

Thanks @jatinkhilnani ! I'll take the dependency issue in a separate thread, but yeah, we're generally overdue for some modernization.

@jatinkhilnani
Copy link
Contributor Author

jatinkhilnani commented Jul 28, 2020

Thank you for the same. I have created another PR #82 for filter deformer, can include modernization (#81) changes in that one or contribute separately as well. Please suggest.

@jatinkhilnani jatinkhilnani mentioned this pull request Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants