KEMBAR78
Lint code style and remove PY2 compatibility by powerexploit · Pull Request #1386 · pytorch/audio · GitHub
Skip to content

Conversation

@powerexploit
Copy link
Contributor

@powerexploit powerexploit commented Mar 12, 2021

Description

Hi 👋 I ran the DeepSource static analyzer on the forked copy of this repo and found some interesting code quality issues. This PR fixes a few of them.

Summary of Fixes

  • Fixed implicit Object Inheritance
  • Remove unnecessary use of comprehension
  • Consider merging these comparisons with 'in'
  • Consider using max

Type of change

  • Antipattern
  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@powerexploit powerexploit force-pushed the fix-bugs branch 2 times, most recently from 424ef55 to bbfb8ce Compare March 12, 2021 07:43
@powerexploit powerexploit marked this pull request as ready for review March 12, 2021 07:43
@powerexploit powerexploit mentioned this pull request Mar 12, 2021
4 tasks
@powerexploit
Copy link
Contributor Author

@mthrok review the updated PR and merge the fixes.

@mthrok
Copy link
Contributor

mthrok commented Mar 15, 2021

@ankitdobhal

This PR contains a different set of changes than #1351.
Can you limit the change to the object inheritance and list and dict update from 41dbfd9?

@powerexploit
Copy link
Contributor Author

@ankitdobhal

This PR contains a different set of changes than #1351.
Can you limit the change to the object inheritance and list and dict update from 41dbfd9?

Thanks @mthrok for pointing out , I have updated the PR with new fixes.

Copy link
Contributor

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

Hi @ankitdobhal

Thanks for the update.

Can you revert the changes in torchaudio/functional/functional.py?
Unlike the other changes, there is no issue in the code and it can impact the TorchScript IR (intermediate representation) when compiled.

@powerexploit powerexploit force-pushed the fix-bugs branch 2 times, most recently from 3451b3e to 48f9c00 Compare March 16, 2021 15:34
- Fixed implicit object inheritance

- Use `max` built-in to get the maximum of two values

- Remove unnecessary use of comprehension
@mthrok mthrok changed the title [Improvements] : Fixed Code Quality Issues Lint code style and remove PY2 compatibility Mar 16, 2021
@mthrok mthrok merged commit 6bad3a6 into pytorch:master Mar 16, 2021
@mthrok
Copy link
Contributor

mthrok commented Mar 16, 2021

Thanks!

@powerexploit
Copy link
Contributor Author

Thanks!

Thanks!

Thanks @mthrok .
I would like to propose something about DeepSource.

Activating analysis will help you to continuously analyze repo for code quality issues and fix commonly occurring issues using the auto-fix feature.

I would like to share some steps to activate DeepSource :

  • Install DeepSource on your repository here.
  • create .deepsource.toml configuration which you can use to configure your analysis settings.
  • Activate analysis here.

@mthrok
Copy link
Contributor

mthrok commented Mar 16, 2021

I would like to propose something about DeepSource.

Thanks for the proposal but adding a new tool for our framework is not something torchaudio can decide on its own. The best way to make it adopted is to propose it in PyTorch repository https://github.com/pytorch/pytorch and get an approval, then we can discuss the adaptation steps.

mthrok pushed a commit to mthrok/audio that referenced this pull request Dec 13, 2022
* Update spatial_transformer_tutorial.py

* Update fgsm_tutorial.py

* Update beginner_source/fgsm_tutorial.py

Co-authored-by: Eli Uriegas <1700823+seemethere@users.noreply.github.com>

Co-authored-by: Eli Uriegas <1700823+seemethere@users.noreply.github.com>
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.

3 participants