-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
590ddef
to
0c391f6
Compare
@anirudh2290 @szha This is ready for review. Tests are passing, but for some reason the deploy stage failed due to a Jenkins error. Rerunning now. |
check_numeric_gradient(norm_sym, [in_data], numeric_eps=epsilon, rtol=1e-2, atol=1e-3) | ||
|
||
# Disable numeric gradient https://github.com/apache/incubator-mxnet/issues/11509 | ||
# # check gradient |
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.
Is this supposed to be commented. Did this not help for #11509 ?
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.
Unfortunately it didn't help. I observed a similar error to #11509 (comment)
template<typename DType> | ||
MSHADOW_XINLINE static void Merge(volatile DType& dst_ssq, volatile DType& dst_scale, volatile DType& src_ssq, volatile DType& src_scale) { // NOLINT(*) | ||
if (dst_scale != 0 && dst_scale >= src_scale) { | ||
dst_ssq = dst_ssq + src_ssq * (src_scale / dst_scale) * (src_scale / dst_scale); |
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.
can you please elaborate on how this expression was obtained
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.
Sure. Remember that we use a scaled sum of squares to compute the L2 norm, to avoid numeric instability caused by the squaring and subsequently taking the square root of very small / large numbers.
For efficient reducing, on GPU multiple reducers compute a reduction of a part of a vector to be reduced. Their result is a scaled sum of squares. To combine the reducers, we must find a common scale for all of them. Following the implementation of Reduce, I choose the largest scale.
Above equation simply rescales the sum of squares of the reducer that currently uses a smaller scale value, such that in the end norm(x) = sqrt(ssq) * scale = dst_scale * sqrt(dst_ssq + src_ssq*src_scale/dst_scale*src_scale_dst_scale) = sqrt(src_scale*src_scale*src_ssq + dst_scale*dst_scale*dst_ssq)
(where we wan't to avoid the right part due to numerical instability; here scale and ssq denote what is written to dst_ssq and dst_scale in above code).
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.
thanks for the explanation!
* Add stable nrm2 Reducer * Prefer scipy.linalg.norm over np.linalg.norm as it is numerically stable * Update mshadow * Add stable reducer merge * Use stable merging of reducers in broadcast_reduce-inl.cuh * Update mshadow * Update tests
Description
The mxnet L2 norm implementation is currently prone to under/overflow when called on NDArrays with very small or large elements. This PR adds a stable L2 norm reducer and switches the dense
norm
operator to make use of it.The implementation follows the Blas nrm2 reference implementation.
As a final rescaling step is necessary at the end of the reduction, this PR also introduces a new
Finalize
method for Reducers. This method is a noop for all previous reducers. A separate PR needs to add theFinalize
method to the maximum and minimum reducers in mshadow.Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
norm()
operatorComments
Finalize
method to all existing reducers (i.e. maximum, minimum, sum, product, nansum, nanprod) using SFINAE but it didn't get it to work immediately (the Finalize() would correctly be skipped if it the respective reducer didn't implement it, but for unknown reasons was also skipped if the reducer implemented it). As there are not many reducers, I added the method for now. The SFINAE approach was as follows: