-
Notifications
You must be signed in to change notification settings - Fork 235
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
[gTest] Add more test coverage for Batchnorm Forward Training #3444
base: develop
Are you sure you want to change the base?
Conversation
INSTANTIATE_TEST_SUITE_P(Smoke, | ||
GPU_BN_FWD_Train_Large_FP64, | ||
testing::Combine(testing::ValuesIn(NetworkSmall<BNTestCase>()), | ||
testing::Combine(testing::ValuesIn(NetworkLarge<BNTestCase>()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall it be a new Full
instance? Let's keep Smoke
tests relatively small, that's the point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @CAHEK7 could you clarify what you mean by "Full" instance? For each data type, there's 4 sets of test data for NetworkSmall and 32 for NetworkLarge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a new Full instance. Its just there was a typo before. It should be calling NetworkLarge for testing AP2. For AP1 we only test small bn network (4 test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean Full
tests and Smoke
tests - Smoke
is a small and fast subset which checks only a basic functionality and must not have any long-running tests or large configs.
The main point of Smoke
tests and a quick sanity check before the long runs.
That's why I'm kind of recommend to move those tests into the Full
tests, since it uses large configs.
INSTANTIATE_TEST_SUITE_P(Smoke, | ||
GPU_BN_FWD_Train_Large_FP64, | ||
testing::Combine(testing::ValuesIn(NetworkSmall<BNTestCase>()), | ||
testing::Combine(testing::ValuesIn(NetworkLarge<BNTestCase>()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a new Full instance. Its just there was a typo before. It should be calling NetworkLarge for testing AP2. For AP1 we only test small bn network (4 test).
More test cases added for better coverage. Here's the summary
N C H W: 768 1 14 14
Covers:
forward_spatial_single.cpp: variant == 0, variant == 1
768 1 14 14.txt
N C H W: 768 1 23 23
Covers:
forward_spatial_single.cpp: variant == 1 and IsApplicable()
forward_spatial_multiple.cpp: variant == 2 (1st)
768 1 23 23.txt
N C H W: 832 1 14 14
Covers:
forward_spatial_single.cpp: variant == 1 and IsApplicable()
forward_spatial_multiple.cpp: variant == 0 then variant == 2 (2nd)
832 1 14 14.txt
N C H W: 832 1 28 28
Covers:
forward_spatial_single.cpp: variant == 1 and IsApplicable()
forward_spatial_multiple.cpp: variant == 2 (1st) then variant == 2 (2nd)
832 1 28 28.txt
A few questions/observations:
Should NetworkLarge be used for GPU_BN_FWD_Train_Large_FP64?
MIOpen/test/gtest/bn_fwd_train.cpp
Line 147 in 310192d
This blurb in IsApplicable
MIOpen/src/solver/batchnorm/forward_spatial_single.cpp
Lines 75 to 80 in 310192d
is the same as in GetSolution
MIOpen/src/solver/batchnorm/forward_spatial_single.cpp
Lines 159 to 161 in 310192d
Which means this snippet will never be run
MIOpen/src/solver/batchnorm/forward_spatial_single.cpp
Lines 169 to 179 in 310192d
Likewise, these two lines are identical
MIOpen/src/solver/batchnorm/forward_spatial_single.cpp
Line 83 in 310192d
MIOpen/src/solver/batchnorm/forward_spatial_single.cpp
Line 182 in 310192d
Due to 2 and 3 above, variant will never be set to 2 in forward_spatial_single.cpp
MIOpen/src/solver/batchnorm/forward_spatial_single.cpp
Lines 75 to 80 in 310192d
And forward_spatial_multiple.cpp will be called. In its GetSolution, this snippet will always be skipped due to the fact it's false as above
MIOpen/src/solver/batchnorm/forward_spatial_multiple.cpp
Lines 123 to 132 in 310192d
Therefore, variant will never be 0 or 1 for forward_spatial_multiple.cpp
Please note the cases that do not pass IsApplicable() in forward_spatial_single.cpp will be handled in IsApplicable() in forward_spatial_multiple.cpp
MIOpen/src/solver/batchnorm/forward_spatial_multiple.cpp
Line 53 in 310192d