-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix flaky test: test_broadcast_binary_op #11875
fix flaky test: test_broadcast_binary_op #11875
Conversation
Could you resolve conflicts? |
Was this broken again? I had fixed it. Sorry I have deja vu now |
# doubles as well. This was a flaky test before when using float32. seed 1688524483, 1768433044 | ||
a = mx.sym.cast(a_, dtype='float64') | ||
b = mx.sym.cast(b_, dtype='float64') | ||
mx.sym.cast(b, dtype='float64') |
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.
extra cast on 1919?
test_bmod is still commented as flaky. |
you added the fix for test_binary_op, test_broadcast_binary_op has the same issue. |
Got it, 👍🏼 can you remove the extra cast? or does it have a purpose / side effect? |
👍🏼(lgtm) |
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.
👍🏼
a = mx.sym.cast(a_, dtype='float64') | ||
b = mx.sym.cast(b_, dtype='float64') | ||
# '%' is sensitive to the precision of the calculation. Force numpy to match mxnet's float32. | ||
#check_binary_op_forward(c, lambda a, b: np.float32(a) % np.float32(b), gen_binary_data) |
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.
Does having the commented check_binary_op call here help reader reason about the rest of the test? Is it needed?
@@ -1911,10 +1911,17 @@ def test_bdiv(a, b): | |||
check_binary_op_forward(c, lambda a, b: a / b, gen_broadcast_data, mx_nd_func=mx.nd.divide) | |||
check_binary_op_backward(c, lambda g_out, a, b: (g_out / b, - g_out * a / (b * b)), gen_broadcast_data) | |||
|
|||
def test_bmod(a, b): | |||
def test_bmod(a_, b_): |
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.
Are you using a and a_ on purpose? It seems like sometimes you are using a and b, while other times you use a_ and b_
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 think a_ and b_ are placeholders here before the sym.cast operator is applied to them and they become a and b. This enforces that they'll be float64s before the broadcast_mod op is applied, which would otherwise cause numerical issues. Then we compare the result of the broadcast_mod (i.e. the c variable) with the python and imperative versions (which are both passed as lambdas to the check).
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.
LGTM, thanks for addressing comments.
* cast inputs to f32 * retrigger * retrigger * remove extra cast * remove commented out function * retrigger
Description
fix flaky test test_operator.py: test_broadcast_binary_op by casting inputs to float32
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments