-
Notifications
You must be signed in to change notification settings - Fork 129
Handle invalid BroadcastTo shape in C backend #175
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
555f2a4
to
43a0a79
Compare
43a0a79
to
df568d9
Compare
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.
Looks good @ricardoV94. I left three comments
8be1bf8
to
3c43f0a
Compare
tests/tensor/test_extra_ops.py
Outdated
@@ -1313,7 +1333,7 @@ def test_memory_leak(self): | |||
blocks_last = blocks_i | |||
|
|||
tracemalloc.stop() | |||
assert np.allclose(np.mean(block_diffs), 0) | |||
assert np.mean(block_diffs) <= (0 + 1e-8) |
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 was failing with negative values, which should not be problematic (?). No idea what the function does when it raises.
3c43f0a
to
883c963
Compare
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 @ricardoV94. I don't know what do make out of the performance regression. I doesn't seem related to this PR but I can't seem to find a way to get the detailed benchmark results from the cache or from this PR.
The last cached benchmark might have been a CI fluke and this one looks bad in comparison? Only the C scan test could possibly have been affected so I'll run it locally before and after the changes to see if it complains. |
Looks like the performance regression problems are happening because the tests that use random are not being seeded, and different seeds can lead to very different run times |
Closes #152