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

feat(concrete_npe): remove torus type generics #144

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

mayeul-zama
Copy link
Contributor

@mayeul-zama mayeul-zama commented Aug 26, 2022

Description

Replace remove torus type generics by a function parameter log2_modulus in all relevant functions in concrete-npe.

  • the generics torus type in only ever used to extract the number of bits, so we don't loose any useful information by only giving the log2_modulus
  • we only ask for the information we need so it's less constraining for callers
  • the generics are contaminating, which forces us to use generics too in the optimizer (log2_modulus -> generic type) is not possible (or very ugly: match log2_modulus {32 => f::(...), 64 => f::(...))
  • for another client of concrete-npe using generics, it's possible to get the log2_modulus from the generic type (genric_type -> log2_modulus) is clean (f(..., T::BITS))
  • we add a function parameter but we also remove a generic type, so we slightly reduce signature complexity
  • with generics, monomorphisation is done (for each type we call generic a function with, the compiler creates a separate version) which may increase compile times and binary size
  • theoretical future advantage: with the log2_modulus, we can call the functions for any number of bits, which may be useful for a FPGA implementation which doesn't use 16, 32, 64 or 128 bits

Checklist

(Use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • The PR description links to the related issue (to link an issue, use '#XXX'.)
  • The tests on AWS have been launched and are successful (comment with @slab-ci cpu_test and/or @slab-ci gpu_test to trigger the tests)
  • The draft release description has been updated
  • Check for breaking changes (including serialization changes) and add them to commit message following the conventional commit specification

@cla-bot
Copy link

cla-bot bot commented Aug 26, 2022

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @mayeul-zama on file. In order for us to review and merge your code, please send an email to hello@zama.ai to get yourself added

@mayeul-zama mayeul-zama force-pushed the npe_remove_torus_type_generics branch from acfa2e2 to 6a27935 Compare August 26, 2022 08:27
@cla-bot cla-bot bot added the cla-signed label Aug 26, 2022
@mayeul-zama
Copy link
Contributor Author

@slab-ci cpu_test

@mayeul-zama mayeul-zama force-pushed the npe_remove_torus_type_generics branch from 6a27935 to 8452560 Compare August 26, 2022 08:39
@mayeul-zama
Copy link
Contributor Author

@slab-ci cpu_test

Copy link
Collaborator

@agnesLeroy agnesLeroy left a comment

Choose a reason for hiding this comment

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

Hi @mayeul-zama! Thank you for the contribution! 🙂 It looks good to me, I just have a small comment you'll see. 😉

fn get_modular_standard_dev<Uint>(&self) -> f64
where
Uint: UnsignedInteger;
fn get_modular_standard_dev(&self, log2_modulus: u64) -> f64;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a small thing: we could use a u32 or even a u8 for log2_modulus I guess, it's always going to be 32 or 64...

Copy link
Member

Choose a reason for hiding this comment

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

I think rust uses u32 for these like when calling e.g. 2u64.pow(/* u32 here */)

@agnesLeroy agnesLeroy force-pushed the npe_remove_torus_type_generics branch from a23e60d to b188e88 Compare August 29, 2022 09:19
BREAKING CHANGE: concrete-commons DispersionParameter interface is
modified
@agnesLeroy agnesLeroy force-pushed the npe_remove_torus_type_generics branch from b188e88 to b9c8bda Compare August 29, 2022 09:23
@agnesLeroy agnesLeroy merged commit a6fb288 into main Aug 29, 2022
@agnesLeroy agnesLeroy deleted the npe_remove_torus_type_generics branch August 29, 2022 11:28
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.

None yet

3 participants