zama-ai/tfhe-rs

Should `integer::CompressedServerKey` have `decompress` method?

matthiasgeihs opened this issue Β· 6 comments

Describe the bug
Not necessarily a bug description, but an observation and a corresponding question.
In the documentation, compressed server key decompression is performed via:

let sks = compressed_sks.decompress();

However, the decompress method is only available on the high level API.
For example, decompress is not available on type integer::CompressedServerKey.
Is this intentional?

To Reproduce

use tfhe::integer::{gen_keys_radix, CompressedServerKey};
use tfhe::shortint::prelude::PARAM_MESSAGE_2_CARRY_2_KS_PBS;
use tfhe::shortint::ClassicPBSParameters;

pub fn main() {
    const PARAMS: ClassicPBSParameters = PARAM_MESSAGE_2_CARRY_2_KS_PBS;
    const NUM_BLOCKS: usize = 4;

    let (ck, sk) = gen_keys_radix(PARAMS, NUM_BLOCKS);
    let sk_compressed = CompressedServerKey::new_radix_compressed_server_key(&ck.into());
    let sk = sk_compressed.decompress(); // no method named `decompress` found for struct `tfhe::integer::CompressedServerKey` in the current scope
}

(Also it's a bit surprising that we need to use ck.into() in combination with new_radix_compressed_server_key even though ck is already a radix key.)

Expected behaviour
Should compile.

Additional context
tfhe = { version = "0.5.0" }

Hello @matthiasgeihs

Some of the APIs are still work in progress, your remark about the decompression primitive is valid and we will add it for sure, we tried to use the Into trait from rust for some of those operations, but it seems it's not discoverable enough.

As for the "into" question, we implement the AsRef trait which should make things a bit less confusing, we have two types of integers and the CRT ones were added late in the original integer design, leaving some API in a subpar state, AsRef is likely the way we will give access to underlying primitives moving forward.

If you think the documentation could be improved we are open source and will gladly take suggestions or contributions once we agree on the format of the documentation πŸ™‚

here is your example revisited:

use tfhe::integer::{CompressedServerKey, RadixClientKey, ServerKey};
use tfhe::shortint::prelude::PARAM_MESSAGE_2_CARRY_2_KS_PBS;
use tfhe::shortint::ClassicPBSParameters;

pub fn main() {
    const PARAMS: ClassicPBSParameters = PARAM_MESSAGE_2_CARRY_2_KS_PBS;
    const NUM_BLOCKS: usize = 4;

    let ck = RadixClientKey::new(PARAM_MESSAGE_2_CARRY_2_KS_PBS, NUM_BLOCKS);
    let sk_compressed = CompressedServerKey::new_radix_compressed_server_key(ck.as_ref());
    let sk: ServerKey = sk_compressed.into(); // no method named `decompress` found for struct
                                              // `tfhe::integer::CompressedServerKey` in the current
                                              // scope
}

Cheers

Cool. Thx. Looks clean, although, as you also said, I find that into conversion and as_ref are less discoverable compared to explicit functions. Anyhow. Thanks for the quick feedback as usual and good to hear that you are aware and potentially reiterating in the future. πŸ‘

of course, we are trying to find a reasonable middle ground between "going fast with features" and "keep features easy to use/discoverable" generally the HL API is the one getting the most attention in that respect !

generally the HL API is the one getting the most attention in that respect !

Good to know. I feel like in the documentation that does not come through all the time. Only in the quick start and the How To the HL API gets some attention but then not anymore in fine grained APIs. I guess that’s intentional, but as a developer I would usually look up the fundamentals first (i.e., api reference) and then reside to how to guides for higher level concepts if needed.

thanks that's valuable feedback

Maybe it's just me though πŸ˜„ πŸ€·β€β™‚οΈ Overall I think the quality of the documentation is already very good πŸ‘ Cheers!