-
Notifications
You must be signed in to change notification settings - Fork 553
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
Use L2Expanded
/L2SqrtExpanded
instead of the *Unexpanded
variants
#6303
base: branch-25.04
Are you sure you want to change the base?
Conversation
It seems that the small numeric changes are making one or a few tests get out of their error margin. We should run some of these tests a number of times (at least 20... more if possible) and see in what margin of error they fall and if it's still an acceptable margin (probably) |
Previously we were using `L2Unexpanded`/`L2SqrtUnexpanded`, which are more costly.
Previously we were using `L2SqrtUnexpanded`, which is more costly.
There are 3 consistent failures, all in 2 are tolerance issues - bumping The 3rd is for If no one is opposed, I think I'll revert the change in |
0529289
to
76d5efb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## branch-25.04 #6303 +/- ##
===============================================
Coverage ? 67.07%
===============================================
Files ? 202
Lines ? 13076
Branches ? 0
===============================================
Hits ? 8771
Misses ? 4305
Partials ? 0 ☔ View full report in Codecov by Sentry. |
This is all green now, should be good-to-go. |
The
*Expanded
variants were already used in most places, but not everywhere. Using the*Expanded
versions should be more performant in all cases, and switching to be uniform across cuml gives more expected behavior.