Skip to content

Conversation

@krishnakalyan3
Copy link
Contributor

@krishnakalyan3 krishnakalyan3 commented Nov 12, 2020

Fixes for #1002

@facebook-github-bot
Copy link
Contributor

Hi @krishnakalyan3!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@krishnakalyan3 krishnakalyan3 changed the title add missing modules [DOC] Add missing modules and minor fixes Nov 12, 2020
@krishnakalyan3
Copy link
Contributor Author

krishnakalyan3 commented Nov 12, 2020

Hello @mthrok, I am not sure what is required here. A function signature update or update to the doc string?

Could you please clarify?.

@krishnakalyan3
Copy link
Contributor Author

The following files have
image

Should all their references be updated?.

@krishnakalyan3
Copy link
Contributor Author

Screen shots:

Screenshot 2020-11-12 at 20 11 24

Screenshot 2020-11-12 at 20 11 04

Screenshot 2020-11-12 at 20 10 33

@mthrok
Copy link
Contributor

mthrok commented Nov 12, 2020

Hi @krishnakalyan3

Thanks for contribution.

Hello @mthrok, I am not sure what is required here. A function signature update or update to the doc string?

Could you please clarify?.

Let's remove self.rng from __init__ and replace speed = self.rng.uniform(0.5, 2.0) with speed = 0.5 + 1.5 * torch.rand()

~~~~~~~~~~~~~~~~~~~~~

.. autofunction:: amplitude_to_DB
.. autofunction:: spectrogram
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like these changes are accidentally introduced.

Could you put DB_to_amplitude next to amplitude_to_DB and put griffinlim next to spectrogram, so that (loosely) related functions are in nearby?

@mthrok
Copy link
Contributor

mthrok commented Nov 12, 2020

The following files have
image

Should all their references be updated?.

Yes, please.

2: warnings
3: details of processing
4-6: increasing levels of debug messages
1. failure messages
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I am unable to get the points into a list even after making this change. Any advice?.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@krishnakalyan3

What about adding hyphen in front?

        - 1: failure
        - 2: details of processing
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I am missing something obvious here.

make html

Does not update the HTML from the python docstrings. Is there a special command to update docs from the docstrings.

I tried

  • deleting build folder
  • building this library from source UILD_SOX=1 MACOSX_DEPLOYMENT_TARGET=10.9 CC=clang CXX=clang++ python setup.py install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @krishnakalyan3

I looked at your issue. I tried it on my end and things worked fine. Maybe it's about the browser cache?
If you still have the problem, you can apply the following patch and move on. (Note that there must be one blank line before the start of the list.

diff --git a/torchaudio/utils/sox_utils.py b/torchaudio/utils/sox_utils.py
index 076e266..3f9ad0b 100644
--- a/torchaudio/utils/sox_utils.py
+++ b/torchaudio/utils/sox_utils.py
@@ -26,10 +26,11 @@ def set_verbosity(verbosity: int):

     Args:
         verbosity (int): Set verbosity level of libsox.
-            1. failure messages
-            2. warnings
-            3. details of processing
-            4-6. increasing levels of debug messages
+
+            * ``1`` failure messages
+            * ``2`` warnings
+            * ``3`` details of processing
+            * ``4``-``6`` increasing levels of debug messages

     See Also:
         http://sox.sourceforge.net/sox.html

@mthrok
Copy link
Contributor

mthrok commented Nov 13, 2020

Screen shots:

Screenshot 2020-11-12 at 20 11 24 Screenshot 2020-11-12 at 20 11 04 Screenshot 2020-11-12 at 20 10 33

If you have extra time, could you also fix the reference of griffinlim?

@krishnakalyan3
Copy link
Contributor Author

@mthrok will do. I am unable to get the changes in the docs. make html does not show me the docs with the changes reflected.

@mthrok
Copy link
Contributor

mthrok commented Nov 16, 2020

Hi @krishnakalyan3

Which part do you mean? Let me know so that I can help you.

@mthrok will do. I am unable to get the changes in the docs. make html does not show me the docs with the changes reflected.

@krishnakalyan3
Copy link
Contributor Author

krishnakalyan3 commented Nov 23, 2020

The following files have
image
Should all their references be updated?.

Yes, please.

Hello @mthrok,
Thank you for your reviews and comments so far. I still have issues viewing documentation correctly. Can I address this issue (point 1: Issues in functional.py ) in a separate PR?. (This way I can have more time trying to figure out the doc Issues I have).

@mthrok
Copy link
Contributor

mthrok commented Dec 4, 2020

Hello @mthrok,
Thank you for your reviews and comments so far. I still have issues viewing documentation correctly. Can I address this issue (point 1: Issues in functional.py ) in a separate PR?. (This way I can have more time trying to figure out the doc Issues I have).

Hi @krishnakalyan3

Sorry for getting back to you late. Yes, we can continue this in another PR. Since this PR contains improvement, I will go ahead and merge it. Thanks for the contribution!

One thing I notice about the current state is that torch.rand() in sox_effects.py does not work. random.randn() is better.

@mthrok mthrok merged commit 2a02d7f into pytorch:master Dec 4, 2020
@krishnakalyan3
Copy link
Contributor Author

@mthrok many thanks :)

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