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

Added test for Alpha Dropout #2367

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Bhavay-2001
Copy link

@Bhavay-2001 Bhavay-2001 commented Dec 30, 2023

Initial pull request for the Issue #1851, stating adding tests for the Alpha Dropout. This PR is based on my intial understanding of the problem. Would love to alter it further to solve the issue.

PR Checklist

  • Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable

Copy link
Member

@darsnack darsnack left a comment

Choose a reason for hiding this comment

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

This looks good. The changes to AlphaDropout normalization tests look good. The newly added test file can just be deleted.

test/layers/normalisation.jl Outdated Show resolved Hide resolved
@darsnack
Copy link
Member

darsnack commented Jan 4, 2024

Can you delete the test/dropout.jl file that you have added?

So far, this PR is on the correct trend, but not targeting the right set of tests to replace. Specifically, we want to replace

# Known good value ranges
# Values taken from https://github.com/pytorch/pytorch/blob/v1.10.0/test/cpp/api/modules.cpp#L1337-L1338
x = ones(100)
if isempty(rng_kwargs)
@test 40 < sum(evalwgrad(m, x)) < 130
else
# FIXME: this breaks spuriously for MersenneTwister
@test_skip 40 < sum(evalwgrad(m, x)) < 130
end
with a statistical test instead of some magic numbers.

In other words, you should pass in a known input (ones(100)). Given that 1 - p (they use q in the paper instead of p) of the input elements get dropped, what should the mean and variance of sum(evalwgrad(m, x)) be? Once you derive those, can you call the layer many times on ones(100) and test that the mean does match the quantity you derived with statistical confidence (i.e. test it using a t-test or z-test)?

@Bhavay-2001
Copy link
Author

Hi @darsnack, the sum(evalwgrad(m, x)) sums up all the gradient values which are returned by the function. However, I have calculated the mean and variance for evalwgrad(m, x). Can you explain me the second half of the message? I need to pass the x=ones(100) input to the AlphaDropout layer and then take the mean and variance of the output which is calculated? And compare that with the mean and variance of evalwgrad(m, x) through t-test or z-test?

@Bhavay-2001
Copy link
Author

Also, the problem is caused by this test.

@test mean(y)  (q*u) + ((1-q)*α′)

I will update the normalisation.jl file with this test as I cannot see it right now. I just want to know where am I going wrong in this?

Copy link
Author

@Bhavay-2001 Bhavay-2001 left a comment

Choose a reason for hiding this comment

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

The test fails here. The part that you suggested has no relation with this code I guess. Rest, please let me know what is wrong here.

@darsnack
Copy link
Member

darsnack commented Jan 6, 2024

That's because that test is testing for the wrong thing. The scaling parameters in the AlphaDropout layer rescale the output so that mean(y) = 0. This is exactly what is tested above the test you added. The test you added fails because it is not a correct test.

The code that I pointed out in the review is the actual lines of code that the original issue is referring to. sum(evalwgrad(m, x)) tests more than just evalwgrad. It is possible for mean(y) to be 0 and var(y) to be 1, and still y is not the output of a dropout layer (e.g. a unit normal distribution's samples has the same mean and variance). The sum with and input of ones allows you to count the number of masked outputs.

@ToucheSir
Copy link
Member

And compare that with the mean and variance of evalwgrad(m, x) through t-test or z-test?

Not evalwgrad(m, x), but some ground truth of what we expect the output distribution to be. That ground truth can't be found anywhere in the existing code, so you'd have to calculate it somehow and then turn that into tests.

@Bhavay-2001
Copy link
Author

Hi @darsnack @ToucheSir, I have prepared a code sample of what I have understood. Please let me know if its correct or what needs to corrected.

x = ones(100)
println(x)
println("MEAN of x: ", mean(x))

m = AlphaDropout(0.4);
println(m)

y = evalwgrad(m, x)
y_ = m(x)
println(y)
println(y_)

println("MEAN of y: ", mean(y))
println("MEAN of y_: ", mean(y_))
println("SUM: ", sum(evalwgrad(m, x)))

@test mean(y)  mean(y_) atol=0.1

@darsnack
Copy link
Member

darsnack commented Jan 7, 2024

First, evalwgrad(m, x) and m(x) produce the same output. The first just computes that output under a gradient context. How they relate to each other, which is what mean(y) = mean(y_) tests, is something that is already tested elsewhere.

We are asking have you look at mean(sum(evalwgrad(m, x)) for _ in 1:1000). What should that be analytically? Write a test that shows the evaluated mean matches what you derive analytically.

An alternative test would be to look not at the mean and variance of evalwgrad(m, x), but apply a goodness of fit test for the distribution of the outputs. This is what @ToucheSir suggested.

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

Successfully merging this pull request may close these issues.

3 participants