-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Fix gradgrad issues with BatchNorm and Advanced Indexing (v.0.2.0) #2268
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
Conversation
…t needed. These could cause a problem with double backwards because they were std::move'd in Backward.
… indexing case that fails check.
|
@killeent for simplicity I fixed the advanced indexing case in python because it's easier, but it should probably be fixed in C++ in case we ever call advanced_indexing_select in other places. Let's do that in the master version; I'd like to keep this v.0.2.0 version as light as possible. |
| std::move(grad_bias)); | ||
| return wrap_outputs(all_inputs, std::move(outputs), [&](FunctionFlags f) { | ||
| return std::make_shared<BatchNormBackwardBackward>( | ||
| f, *this, std::move(save_mean), std::move(save_std), |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| jacobian_x[:, i].zero_() | ||
| else: | ||
| jacobian_x[:, i] = d_x.to_dense() if d_x.is_sparse else d_x | ||
| for jacobian_c in (jacobian, jacobian_reentrant): |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This PR is to fix https://ontrack-internal.amd.com/browse/SWDEV-534855: test_cuda.py::TestCuda::test_hip_device_count fails on one gpu machine. Needs to cherry-pick it to upstream as well once this PR is merged.
This adds two tests for gradchecks:
That backward is reentrant
That input grad sizes match input sizez.
Uncovers an issue with BatchNorm which is now fixed
Uncovers an issue with advanced indexing which is also now fixed.