Skip to content
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

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

IceTDrinker
Copy link
Member

@IceTDrinker IceTDrinker commented Oct 22, 2024

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

@cla-bot cla-bot bot added the cla-signed label Oct 22, 2024
@IceTDrinker IceTDrinker marked this pull request as ready for review October 23, 2024 11:11
@IceTDrinker
Copy link
Member Author

I launched the tests in advance and as you can see all green, so the fix is behaving properly

@IceTDrinker
Copy link
Member Author

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

@IceTDrinker
Copy link
Member Author

tested my bit trick for input values, the bit trick agrees with the plain if, will check the one in the decomposition as well,

@IceTDrinker
Copy link
Member Author

Ok will push a test validating the bit trick, then once this hits the PR GPU team can look at updating their code

@IceTDrinker
Copy link
Member Author

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

@IceTDrinker IceTDrinker force-pushed the am/fix/balanced-decomposition-1-level branch from 2168a44 to f329099 Compare October 24, 2024 09:59
@zama-bot zama-bot removed the approved label Oct 24, 2024
@IceTDrinker IceTDrinker force-pushed the am/fix/balanced-decomposition-1-level branch from f329099 to 9e7173a Compare October 24, 2024 10:01
@IceTDrinker IceTDrinker force-pushed the am/fix/balanced-decomposition-1-level branch from 9e7173a to d38d46f Compare October 24, 2024 10:49
@zama-bot zama-bot removed the approved label Oct 24, 2024
@IceTDrinker
Copy link
Member Author

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
@IceTDrinker IceTDrinker force-pushed the am/fix/balanced-decomposition-1-level branch from d38d46f to fc6ed1d Compare October 24, 2024 13:18
@zama-bot zama-bot removed the approved label Oct 24, 2024
@IceTDrinker
Copy link
Member Author

latest pbs 128 failure (mismatch between two implems we have) is now fixed

Copy link
Contributor

@mayeul-zama mayeul-zama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@IceTDrinker IceTDrinker merged commit c650475 into main Oct 25, 2024
87 checks passed
@IceTDrinker IceTDrinker deleted the am/fix/balanced-decomposition-1-level branch October 25, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants