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.

@mayeul-zama
Copy link
Contributor Author

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

@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)

@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