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

Fix return dtype in MVDR module #2376

Closed
wants to merge 3 commits into from
Closed

Conversation

nateanl
Copy link
Member

@nateanl nateanl commented May 10, 2022

Address #2375
The MVDR module internally transforms the dtype of complex tensors to torch.complex128 for computation and transforms it back to the original dtype before returning the Tensor. However, it didn't convert back successfully due to specgram_enhanced.to(dtype), which should be specgram_enhanced = specgram_enhanced.to(dtype). Fix it to make the output dtype consistent with original input.

@facebook-github-bot
Copy link
Contributor

@nateanl has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Collaborator

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

Can we have corresponding tests?

@nateanl nateanl requested a review from mthrok May 10, 2022 17:04
@nateanl nateanl requested a review from mthrok May 10, 2022 17:37
@facebook-github-bot
Copy link
Contributor

@nateanl has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Collaborator

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

Looks good. BTW can we move the implementation of MVDR into dedicated file, such as torchaudio/transforms/_mvdr.py?

@nateanl
Copy link
Member Author

nateanl commented May 10, 2022

Looks good. BTW can we move the implementation of MVDR into dedicated file, such as torchaudio/transforms/_mvdr.py?

Sure, I prefer using _beamforming.py or _multi_channel.py as there are other beamforming methods liks GEV or WPD, and other multi-channel methods like WPE, IPD, etc.

Or you mean for this specific MVDR module for deprecation purpose?

@mthrok
Copy link
Collaborator

mthrok commented May 10, 2022

Looks good. BTW can we move the implementation of MVDR into dedicated file, such as torchaudio/transforms/_mvdr.py?

Sure, I prefer using _beamforming.py or _multi_channel.py as there are other beamforming methods liks GEV or WPD, and other multi-channel methods like WPE, IPD, etc.

Or you mean for this specific MVDR module for deprecation purpose?

What I meant was to put all the multi channel transforms to dedicated script.
_transforms.py is getting huge, but IIUC, multi-channel transforms are not logically connected to the rest of transforms.
Putting them in dedicated file should yield better decoupling.
You can pick the name of the file, as long as it represents the module well.

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.

4 participants