Skip to content

loss must be avg when BS>1 when calling evaluate_batch()#1744

Merged
ycemsubakan merged 2 commits into
speechbrain:developfrom
sangeet2020:WHAMR_bug
Dec 8, 2022
Merged

loss must be avg when BS>1 when calling evaluate_batch()#1744
ycemsubakan merged 2 commits into
speechbrain:developfrom
sangeet2020:WHAMR_bug

Conversation

@sangeet2020

@sangeet2020 sangeet2020 commented Dec 4, 2022

Copy link
Copy Markdown
Contributor

Hi @ycemsubakan ,

I initiate this PR to fix a bug in the WHAMandWHAMR recipe. I realized that the current recipe works fine on batch_size as 1. However, when increased, losses are not averaged when evaluate_batch is called.
Basically, it makes sense to write

loss = self.compute_objectives(predictions, targets)
loss = torch.mean(loss)   # Avg losses in a batch

Thank You

@mravanelli

mravanelli commented Dec 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@ycemsubakan could you please take a look? It might be that the same applies to other recipes (e.g, WSJ2MIX, LibriMix, etc.)

@ycemsubakan

ycemsubakan commented Dec 4, 2022

Copy link
Copy Markdown
Collaborator

@mravanelli @sangeet2020 I will try to do it this week.

@MartinKocour

MartinKocour commented Dec 7, 2022

Copy link
Copy Markdown
Collaborator

@sangeet2020 take a look into mine LibriCSS recipe. IMHO, it should be done inside compute_objective
LibriCSS recipe is here:
https://github.com/MartinKocour/speechbrain/blob/49ab1668c8e8952cf3f33efa3d8d1f15cd3f42a4/recipes/LibriCSS/separation/train.py#L139

@ycemsubakan

ycemsubakan commented Dec 7, 2022

Copy link
Copy Markdown
Collaborator

@MartinKocour , thanks for pointing this out. I 'll take a look tonight.

@sangeet2020

sangeet2020 commented Dec 7, 2022

Copy link
Copy Markdown
Contributor Author

@sangeet2020 take a look into mine LibriCSS recipe. IMHO, it should be done inside compute_objective LibriCSS recipe is here: https://github.com/MartinKocour/speechbrain/blob/49ab1668c8e8952cf3f33efa3d8d1f15cd3f42a4/recipes/LibriCSS/separation/train.py#L139

yes, this is a suitable choice. I will modify it and re-commit. thanks @MartinKocour

@ycemsubakan

ycemsubakan commented Dec 7, 2022

Copy link
Copy Markdown
Collaborator

Hmm... we need make sure it works with both frequency domain and time.. I am checking now..

@ycemsubakan

ycemsubakan commented Dec 7, 2022

Copy link
Copy Markdown
Collaborator

Actually, @MartinKocour , @sangeet2020 doing the averaging inside compute_objectives is not a good option. If we do not we can not individually threshold with respect to loss values. I think @sangeet2020 ' s original solution doesn't break anything. I am still looking at it.

@ycemsubakan

ycemsubakan commented Dec 7, 2022

Copy link
Copy Markdown
Collaborator

I pushed a small cosmetic update. @sangeet2020 , are you planning to have more updates? If yes, we can keep this PR open. If not, we can merge.

@sangeet2020

sangeet2020 commented Dec 8, 2022

Copy link
Copy Markdown
Contributor Author

Hi Cem,
I dont plan any more updates on this PR, I think we are safe to close and merge it.

@ycemsubakan

ycemsubakan commented Dec 8, 2022

Copy link
Copy Markdown
Collaborator

Hi Cem, I dont plan any more updates on this PR, I think we are safe to close and merge it.

Alright, thanks for the PR @sangeet2020!

@ycemsubakan ycemsubakan merged commit 5df3885 into speechbrain:develop Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

4 participants

@sangeet2020 @mravanelli @ycemsubakan @MartinKocour