-
Notifications
You must be signed in to change notification settings - Fork 155
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
fix(core): fix decomposition over 1 level to be balanced #1711
Conversation
I launched the tests in advance and as you can see all green, so the fix is behaving properly |
while checking with @mayeul-zama it was not clear the bit trick was doing exactly what we wanted, will take a look to fix this and potentially do the same on the decompose_one_level function |
tested my bit trick for input values, the bit trick agrees with the plain if, will check the one in the decomposition as well, |
Ok will push a test validating the bit trick, then once this hits the PR GPU team can look at updating their code |
updated the code, the bit tricks are now in a function marked inline(always) and have a dedicated test to verify their output is correct good to review, I'll launch the tests directly with approved |
2168a44
to
f329099
Compare
f329099
to
9e7173a
Compare
9e7173a
to
d38d46f
Compare
something going wrong in the pbs 128, need to check what it is |
- update test_split_pbs to have more iterations as the new decomposition did not trigger a mismatch between both implementations in all cases only running the test once, mismatch is now fixed
d38d46f
to
fc6ed1d
Compare
latest pbs 128 failure (mismatch between two implems we have) is now fixed |
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!
It was brought to our attention that a decomposition over 1 level was not properly balanced and could have a mean value of 0.5 which can be an issue when being fed in an FFT (generating harmonics in the process) this fixes that.
A tests has been added to check on a selected 1 level decomposition that the average of the values returning during decomposition is ~0
cc @agnesLeroy and maybe @guillermo-oyarzun so that you can check how to update this on your end, I can explain the formula in more detail, but normally the comment in the code explains how it works