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

Improve error robustness of unit tests #5535

Merged
merged 10 commits into from
Nov 8, 2023
Merged

Conversation

Emrys365
Copy link
Collaborator

@Emrys365 Emrys365 commented Nov 7, 2023

What?

This PR is a followup PR of #5523 to improve two unit tests to avoid occasional errors caused by numerical precision or randomness.

Why?

Sometimes we may observe errors as in https://github.com/espnet/espnet/actions/runs/6719461822/job/18261148245 which are essentially numerical issues. After the modifications in this PR, these errors should be avoided in most cases.

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #5535 (5ec639f) into master (eb6f93a) will decrease coverage by 0.25%.
Report is 14 commits behind head on master.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #5535      +/-   ##
==========================================
- Coverage   75.43%   75.19%   -0.25%     
==========================================
  Files         711      711              
  Lines       65757    65757              
==========================================
- Hits        49607    49449     -158     
- Misses      16150    16308     +158     
Flag Coverage Δ
test_configuration_espnet2 ∅ <ø> (∅)
test_integration_espnet1 63.00% <66.66%> (-2.68%) ⬇️
test_integration_espnet2 48.61% <66.66%> (+0.10%) ⬆️
test_python_espnet1 19.28% <50.00%> (+0.25%) ⬆️
test_python_espnet2 51.39% <75.00%> (-0.22%) ⬇️
test_utils 22.19% <66.66%> (-0.91%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
espnet/nets/pytorch_backend/frontends/frontend.py 93.75% <100.00%> (ø)
espnet/nets/pytorch_backend/frontends/dnn_wpe.py 0.00% <0.00%> (-97.06%) ⬇️

... and 4 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@sw005320
Copy link
Contributor

sw005320 commented Nov 7, 2023

Thanks, but we still have an issue...

@sw005320 sw005320 added Bugfix CI Travis, Circle CI, etc labels Nov 7, 2023
@sw005320 sw005320 added this to the v.202312 milestone Nov 7, 2023
@Emrys365
Copy link
Collaborator Author

Emrys365 commented Nov 7, 2023

This is weird. I cannot reproduce the error with the same PyTorch version...

@sw005320
Copy link
Contributor

sw005320 commented Nov 7, 2023

Is this operation actually used?

@Emrys365
Copy link
Collaborator Author

Emrys365 commented Nov 7, 2023

Are you talking about the DefaultFrontend with WPE and beamforming? I think we usually use the S2T function (SE+ASR) instead of ASR only, where the new NeuralBeamformer (espnet2/enh/separator/neural_beamformer.py) is used.

We still use the DefaultFrontend for FBank feature extraction, but usually not with beamforming.

@sw005320
Copy link
Contributor

sw005320 commented Nov 7, 2023

I mean the matrix inverse function, not wpe or beamformer.
I remember that there are several implementations for the complex matrix inverse operation.

@Emrys365
Copy link
Collaborator Author

Emrys365 commented Nov 7, 2023

Because it is using the pytorch_wpe package for WPE (where the CI error occurs), the matrix inverse operation is conducted based on torch_complex. And yes, the error message shows the torch_complex package is used for matrix inverse. Both packages are not maintained in our codebase.

@sw005320
Copy link
Contributor

sw005320 commented Nov 7, 2023

Got it.
Is there a way to avoid the issue of going through torch_complex by copying them inside espnet and using the other matrix inverse in the future?

I want to remove such dependencies in the future.

@Emrys365
Copy link
Collaborator Author

Emrys365 commented Nov 8, 2023

I think so. It is what I did before, and probably we can just replace

from espnet.nets.pytorch_backend.frontends.dnn_beamformer import DNN_Beamformer
from espnet.nets.pytorch_backend.frontends.dnn_wpe import DNN_WPE

with the new implementations in espnet2

from espnet2.enh.layers.dnn_beamformer import DNN_Beamformer
from espnet2.enh.layers.dnn_wpe import DNN_WPE

@sw005320
Copy link
Contributor

sw005320 commented Nov 8, 2023

Sounds good!
With this, can we remove these dependencies?
If so, we may consider it in the other PRs. This will simplify things.

@mergify mergify bot added the ESPnet1 label Nov 8, 2023
@Emrys365
Copy link
Collaborator Author

Emrys365 commented Nov 8, 2023

I think at least we can remove pytorch_wpe. It was only used in espnet/nets/pytorch_backend/frontends/dnn_wpe.py. I have verified locally that replacing espnet1 implementations with espnet2 ones can pass the tests.

@sw005320
Copy link
Contributor

sw005320 commented Nov 8, 2023

Nice!
This is a good part of having a unit test!
We can safely replace it :)

@Emrys365
Copy link
Collaborator Author

Emrys365 commented Nov 8, 2023

torch_complex is still now used in Enh since some complex-valued functions have not been implemented yet in torch.

@sw005320
Copy link
Contributor

sw005320 commented Nov 8, 2023

OK, it passes the test!
Can you also remove pytorch_wpe, then?

@Emrys365
Copy link
Collaborator Author

Emrys365 commented Nov 8, 2023

Sure.

@mergify mergify bot added the Installation label Nov 8, 2023
@sw005320
Copy link
Contributor

sw005320 commented Nov 8, 2023

Can you remove it at https://github.com/espnet/espnet/blob/master/setup.py#L42?

@Emrys365
Copy link
Collaborator Author

Emrys365 commented Nov 8, 2023

Yes, I already removed it in the latest commit.

@sw005320 sw005320 merged commit 0d0ab98 into espnet:master Nov 8, 2023
24 of 25 checks passed
@sw005320
Copy link
Contributor

sw005320 commented Nov 8, 2023

Thanks a lot!
This was a difficult bug, but it brought an outcome to remove the old dependency!
Very good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants