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

refactor(integer): add compression key types #1431

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

mayeul-zama
Copy link
Contributor

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jul 29, 2024
@mayeul-zama mayeul-zama force-pushed the mz/integer_compression_key branch from dcf3c11 to 73acc67 Compare August 2, 2024 09:01
@mayeul-zama
Copy link
Contributor Author

To merge after #1437
Will need to add a new intermediate versioned type

@mayeul-zama mayeul-zama force-pushed the mz/integer_compression_key branch from 73acc67 to cb3a793 Compare August 2, 2024 09:45
@mayeul-zama mayeul-zama force-pushed the mz/integer_compression_key branch from cb3a793 to 1f55d61 Compare August 19, 2024 13:23
@mayeul-zama mayeul-zama force-pushed the mz/integer_compression_key branch from 1f55d61 to 2e6e49c Compare August 19, 2024 13:38
@IceTDrinker
Copy link
Member

some GPU test crash related to the change at the moment it seems

Copy link
Contributor

@nsarlin-zama nsarlin-zama left a comment

Choose a reason for hiding this comment

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

Apart from the CI break, the code looks good to me.

Maybe we should add a client key with compression to the data tests because I am not sure that this is properly tested right now (the client key comes from 0.6 and does not have compression)

@mayeul-zama mayeul-zama force-pushed the mz/integer_compression_key branch from 2e6e49c to bd2a680 Compare August 21, 2024 16:28
@IceTDrinker
Copy link
Member

can you check if there are test cases for compression in the data repo ?

Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

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

Small things

radix_cks.new_cuda_compression_decompression_keys(&private_compression_key, &streams);
radix_cks.new_cuda_compression_decompression_keys(&private_compression_key.0, &streams);
Copy link
Member

Choose a reason for hiding this comment

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

I would move the .0 in the cuda code and make sure this interface uses an integer key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


#[derive(Clone, Debug, Serialize, Deserialize, Versionize)]
#[versionize(CompressionPrivateKeysVersions)]
pub struct CompressionPrivateKeys(pub crate::shortint::list_compression::CompressionPrivateKeys);
Copy link
Member

@IceTDrinker IceTDrinker Aug 23, 2024

Choose a reason for hiding this comment

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

this is not consistent with other keys, they are struct with named fields, see e.g. tfhe/src/integer/server_key/mod.rs:36

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced by a field named key

@mayeul-zama mayeul-zama added the data_PR This is a PR that needs to fetch new data for backward compat tests label Sep 6, 2024
@mayeul-zama mayeul-zama merged commit c0d9839 into main Sep 11, 2024
87 of 89 checks passed
@mayeul-zama mayeul-zama deleted the mz/integer_compression_key branch September 11, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved cla-signed data_PR This is a PR that needs to fetch new data for backward compat tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants