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: degree for min max #1800

Merged
merged 1 commit into from
Nov 26, 2024
Merged

Conversation

Tuditi
Copy link
Contributor

@Tuditi Tuditi commented Nov 20, 2024

PR content/description

Contains a test to recreate an issue we have been having at Belfort. The min - max functions seem to fail at the assertion that the carries are empty. IIUC, this is due to the fact that unchecked_add will add together the degrees, but we know that either the LHS or the RHS will be set to 0 after performing the LUT here.

It can be solved in the following ways:

  1. Set the degree to Degree::new(msg_modulus - 1).
  2. Add a remainder operation in the LUTs (% message_modulus).
  3. Calculate the maximum degree before applying the additions and the LUTs.
  • Which one is your preferred solution?

Check-list:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Relevant issues are marked as resolved/closed, related issues are linked in the description
  • Check for breaking changes (including serialization changes) and add them to commit message following the conventional commit specification

Copy link

cla-bot bot commented Nov 20, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Tuditi.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

2 similar comments
Copy link

cla-bot bot commented Nov 20, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Tuditi.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

Copy link

cla-bot bot commented Nov 20, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Tuditi.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@tmontaigu
Copy link
Contributor

While the fix to set the degree is correct and should be kept, you are not using the correct test

This makes you believe the function is incorrect but it is.
You are putting an unchecked_ function to a function designed to test a "default" so test gives inputs that are not meant to be handled by an unchecked function and the test also does post condition checks that are not meant for unchecked function.

Also you should use the _parallelized version, its faster and more efficient

tfhe/src/integer/server_key/radix/tests.rs Outdated Show resolved Hide resolved
tfhe/src/integer/server_key/radix/tests.rs Outdated Show resolved Hide resolved
@Tuditi Tuditi changed the title Fix/degree for min max Fix: degree for min max Nov 20, 2024
@Tuditi Tuditi force-pushed the fix/degree-for-min-max branch from ff15728 to 35d74b4 Compare November 20, 2024 14:07
@cla-bot cla-bot bot added the cla-signed label Nov 20, 2024
@Tuditi Tuditi force-pushed the fix/degree-for-min-max branch 2 times, most recently from 13cd230 to d640dce Compare November 20, 2024 14:12
@Tuditi Tuditi marked this pull request as ready for review November 20, 2024 14:13
@Tuditi Tuditi requested a review from tmontaigu November 20, 2024 14:13
@tmontaigu
Copy link
Contributor

Here also you have to do a make fmt and fix any make clippy_integer errors

@Tuditi Tuditi force-pushed the fix/degree-for-min-max branch from d640dce to a12fc53 Compare November 21, 2024 10:40
@Tuditi
Copy link
Contributor Author

Tuditi commented Nov 21, 2024

Here also you have to do a make fmt and fix any make clippy_integer errors

Done :)

@tmontaigu tmontaigu merged commit a1f681e into zama-ai:main Nov 26, 2024
43 of 51 checks passed
@Tuditi Tuditi deleted the fix/degree-for-min-max branch November 27, 2024 08:12
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.

2 participants