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

Conv-TasNet Cumulative Layer Norm Bug? #101

Closed
michelg10 opened this issue Nov 22, 2021 · 4 comments
Closed

Conv-TasNet Cumulative Layer Norm Bug? #101

michelg10 opened this issue Nov 22, 2021 · 4 comments

Comments

@michelg10
Copy link

shouldn't lines 78-92 be

`
step_sum = input.sum(dim=1) # -> (batch_size, T)
cum_sum = torch.cumsum(step_sum, dim=1) # -> (batch_size, T)

cum_num = torch.arange(C, C*(T+1), C, dtype=torch.float) # -> (T, ): [C, 2C, ..., TC]
cum_mean = cum_sum / cum_num # (batch_size, T)
cum_var = (cum_sum - cum_mean)**2/cum_num

cum_mean = cum_mean.unsqueeze(dim=1)
cum_var = cum_var.unsqueeze(dim=1)

output = (input - cum_mean) / (torch.sqrt(cum_var) + eps) * self.gamma + self.beta
`

according to the Conv-TasNet paper?

@tky823
Copy link
Owner

tky823 commented Nov 22, 2021

My CumulativeLayerNorm1d is based on official implementation. You can compare it with mine here.
If there is any possibility that I have misunderstood something, let me know in more detail.

@michelg10
Copy link
Author

compared to the official implementation, your code writes cum_var=cum_squared_mean - cum_mean**2 while the official code writes cum_var = (cum_pow_sum - 2cum_meancum_sum) / entry_cnt + cum_mean.pow(2).

then cum_squared_mean should = (cum_pow_sum - 2cum_meancum_sum) / entry_cnt
In your code cum_squared_mean=cum_squared_sum/cum_num. as entry_cnt=cum_sum,
cum_squared_sum should = cum_pow_sum-2cum_meancum_sum

however cum_squared_sum is defined as torch.cumsum(step_pow_sum, dim=1) which equals cum_pow_sum in the official implementation, so you're missing 2cum_meancum_sum.

am I missing anything here? or is this omitted for a speed / accuracy tradeoff?

@tky823
Copy link
Owner

tky823 commented Nov 23, 2021

formula
formula
formula
formula
formula
formula

In official implementation:
formula

In my repo:
formula
formula

I renamed some variables in check_layer_norm.ipynb at #101 (comment) for readability.

@michelg10
Copy link
Author

My bad! thank you for clearing it up!

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

No branches or pull requests

2 participants