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.

Sign up for GitHub

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

Jump to bottom

Allow extra dimensions with extent 1 in Spectrogram operator & AudioDecoder changes #1679

Merged
merged 3 commits into from
Jan 24, 2020
Merged

Allow extra dimensions with extent 1 in Spectrogram operator & AudioDecoder changes #1679

merged 3 commits into from
Jan 24, 2020

Conversation

jantonguirao
Copy link
Contributor

Signed-off-by: Joaquin Anton janton@nvidia.com

Why we need this PR?

Pick one, remove the rest

  • It adds new feature needed to handle empty dimensions in Spectrogram operator

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

  • What solution was applied:
    • In Spectrogram operator, check that the input shape is 1-D or only one dimension of the shape has an extent greater than 1. If so, we can squeeze the shape into a 1D tensor
  • Affected modules and functionalities:
    • Spectrogram operator
  • Key points relevant for the review:
    • Changes in Spectrogram
  • Validation and testing:
    • Python tests
  • Documentation (including examples):
    • Updated docstr

JIRA TASK: [Use DALI-XXXX or NA]

@jantonguirao jantonguirao requested a review from a team January 21, 2020 12:04
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1083375]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1083381]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1083375]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1083381]: BUILD PASSED

@szalpal
Copy link
Member

szalpal commented Jan 21, 2020

I'm gonna improve this PR with AudioDecoder stuff, please don't merge yet

@szalpal
Copy link
Member

szalpal commented Jan 21, 2020

!build

@szalpal szalpal changed the title Allow extra dimensions with extent 1 in Spectrogram operator Allow extra dimensions with extent 1 in Spectrogram operator & AudioDecoder changes Jan 21, 2020
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1083750]: BUILD STARTED

@JanuszL
Copy link
Contributor

JanuszL commented Jan 21, 2020 •
edited
Loading

Can we add some test: FileReader->AudioDecoder->Spectrogram test to see if that works?

@szalpal
Copy link
Member

szalpal commented Jan 21, 2020

Can we add some test: FileReader->AudioDecoder->Scectogram test to see if that works?

Upssss, forgot about it, adding!

JanuszL
JanuszL reviewed Jan 21, 2020
View reviewed changes
dali/test/python/test_audiodecoder_spectogram.py Outdated
@@ -0,0 +1,115 @@
# Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to run this test as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just make it as another case in the existing test_operator_spectrogram.py ?

JanuszL
JanuszL reviewed Jan 21, 2020
View reviewed changes
dali/test/python/test_audiodecoder_spectogram.py Outdated
out = np.abs(
librosa.stft(y=input_data, n_fft=nfft, hop_length=win_step, window=hann_win)) ** 2

# Alternative way to calculate the spectrogram:
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be removed

Copy link
Member

Choose a reason for hiding this comment

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

Done

@szalpal
Copy link
Member

szalpal commented Jan 21, 2020

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1084339]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1084339]: BUILD FAILED

@szalpal
Copy link
Member

szalpal commented Jan 22, 2020

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1085295]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1085295]: BUILD PASSED

klecki
klecki reviewed Jan 22, 2020
View reviewed changes
dali/test/python/test_audiodecoder_spectogram.py Outdated
read, _ = self.input()
audio, rate = self.decode(read)
spec = self.fft(audio)
return spec
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also return the data from decode and validate their shapes?

Copy link
Member

Choose a reason for hiding this comment

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

That's already tested, here

klecki
klecki requested changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@klecki klecki left a comment

Choose a reason for hiding this comment

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

Minor nitpicks written above, also please adjust the test file name as Janusz suggests.

@JanuszL JanuszL self-requested a review January 23, 2020 01:02
@szalpal
Copy link
Member

szalpal commented Jan 23, 2020

test_operator_spectogram uses random artificial data, I use real data with FileReader here. I'd need to add another 2 pipelines there. Ofc that test there could be changed, but it's out of scope of this PR. IMHO it's better to just add another file

@JanuszL
Copy link
Contributor

JanuszL commented Jan 23, 2020

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1087534]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1087534]: BUILD FAILED

@jantonguirao
Copy link
Contributor Author

test_operator_spectogram uses random artificial data, I use real data with FileReader here. I'd need to add another 2 pipelines there. Ofc that test there could be changed, but it's out of scope of this PR. IMHO it's better to just add another file

I'd simply add those as another set of test cases in test_operator_spectrogram (as Janusz suggested)

@jantonguirao
Allow extra dimensions with extent 1 in Spectrogram operator
1bd6dc7 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1087534]: BUILD PASSED

@szalpal
Changes in Decoder and decoder's test
0835b12 
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
@szalpal
updating spectogram test to cover decoder also
2a476ae 
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
@szalpal
Copy link
Member

szalpal commented Jan 23, 2020

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1088023]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1088023]: BUILD PASSED

@szalpal szalpal merged commit c85420c into NVIDIA:master Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szalpal szalpal szalpal left review comments

@klecki klecki klecki approved these changes

@JanuszL JanuszL JanuszL approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jantonguirao @dali-automaton @szalpal @JanuszL @klecki