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

Get rid of typedefs/SignalInfo and replace AudioMetaData #761

Merged
merged 1 commit into from Jul 8, 2020

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Jul 1, 2020

This PR removes typedefs.[h|cpp], moves SignalInfo from torchaudio namespace to torchaudio::sox_io, removes dummy module from Python interface, then replace info return type with AudioMetaData.

torchaudio::sox_io::SignalInfo struct was expected be data exchange structure between Python and C++ for all info, load and save function of sox_io backend. However, as I simplify the function signature of save and load, they end up not needing such structure, and now info function is the only function that use torchaudio::sox_io::SignalInfo struct.

On Python API level, we can replace this struct with simple Python class while keeping Torchscript-ability. This removes the necessity to introduce dummy module because we no longer need to annotate info function with torchaudio::sox_io::SignalInfo.

@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #761 into master will increase coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #761      +/-   ##
==========================================
+ Coverage   89.16%   89.35%   +0.19%     
==========================================
  Files          32       32              
  Lines        2566     2556      -10     
==========================================
- Hits         2288     2284       -4     
+ Misses        278      272       -6     
Impacted Files Coverage Δ
torchaudio/extension/extension.py 100.00% <ø> (+20.00%) ⬆️
torchaudio/backend/sox_io_backend.py 78.12% <100.00%> (+5.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad7f43f...c9eed5b. Read the comment docs.

@mthrok mthrok marked this pull request as ready for review July 1, 2020 23:25
@mthrok mthrok requested a review from vincentqb July 1, 2020 23:26
Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines -35 to +38
assert info.get_sample_rate() == sample_rate
assert info.get_num_frames() == sample_rate * duration
assert info.get_num_channels() == num_channels
assert info.sample_rate == sample_rate
assert info.num_frames == sample_rate * duration
assert info.num_channels == num_channels
Copy link
Contributor

Choose a reason for hiding this comment

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

The functions are still in there so the change is not BC breaking, right? Is there a reason why you are updating the tests then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The functions are still in there so the change is not BC breaking, right?

No. These get_* functions are removed so this is BC breaking.

Copy link
Contributor

Choose a reason for hiding this comment

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

If a user tries to use them, what will happen? I don't see any instructions to the user on what to do?

@vincentqb vincentqb changed the title Get rid of typedefs/SignalInfo and replace AudioMetaData [BC Breaking] Get rid of typedefs/SignalInfo and replace AudioMetaData Jul 7, 2020
Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

Let's make sure we have the warnings in place to let the users know what to do if they encounter errors for missing operations.

@mthrok
Copy link
Collaborator Author

mthrok commented Jul 7, 2020

Let's make sure we have the warnings in place to let the users know what to do if they encounter errors for missing operations.

That approach is good for stable released feature, but considering the product lifecycle of "sox_io" backed, which is still under heavy development, I do not think that really fits.

@vincentqb
Copy link
Contributor

Let's make sure we have the warnings in place to let the users know what to do if they encounter errors for missing operations.

That approach is good for stable released feature, but considering the product lifecycle of "sox_io" backed, which is still under heavy development, I do not think that really fits.

What I meant above when I asked if something was BC-breaking was whether this was changing the existing "sox" (not "sox_io") interface. "sox_io" has not been part of a release yet, so we don't need to add warnings when we are changing the interface.

  • If this is not changing "sox" (and only changing "sox_io"), then there is no need for warnings and we can proceed.
  • If this is changing "sox", then this is BC-breaking of a feature that was already in a torchaudio release, and we do need a warning.

Can you clarify the response?

@mthrok
Copy link
Collaborator Author

mthrok commented Jul 8, 2020

What I meant above when I asked if something was BC-breaking was whether this was changing the existing "sox" (not "sox_io") interface. "sox_io" has not been part of a release yet, so we don't need to add warnings when we are changing the interface.

This changes the function signature of sox_io backend and has nothing to do with sox backend.

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying :) LGTM

@vincentqb vincentqb changed the title [BC Breaking] Get rid of typedefs/SignalInfo and replace AudioMetaData Get rid of typedefs/SignalInfo and replace AudioMetaData Jul 8, 2020
@mthrok mthrok merged commit 180ede8 into pytorch:master Jul 8, 2020
@mthrok mthrok deleted the replace-signal-info branch July 8, 2020 16:21
@mthrok
Copy link
Collaborator Author

mthrok commented Jul 8, 2020

thanks

@mthrok mthrok restored the replace-signal-info branch July 8, 2020 16:33
@mthrok mthrok deleted the replace-signal-info branch July 14, 2020 16:49
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.

None yet

2 participants