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

function executor in string tests #1818

Merged
merged 4 commits into from
Dec 4, 2024
Merged

Conversation

mayeul-zama
Copy link
Contributor

No description provided.

impl<'a, F> FunctionExecutor<&'a mut RadixCiphertext, RadixCiphertext> for CpuFunctionExecutor<F>
/// For unary operations
///
/// Note, we need to `NotTuple` constraint to avoid conflicts with binary or ternary operations
Copy link
Member

Choose a reason for hiding this comment

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

can you show an example of a conflict case ?

Copy link
Contributor

Choose a reason for hiding this comment

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

impl<F, I1, O> FunctionExecutor<I1, O> for CpuFunctionExecutor<F>
where
    F: Fn(&ServerKey, I1) -> O,
    I1: NotTuple,
{
    fn execute(&mut self, input: I1) -> O {
        let sks = self.sks.as_ref().expect("setup was not properly called");
        (self.func)(sks, input)
    }
}

conflicts with

impl<F, I1, I2 O> FunctionExecutor<I1, O> for CpuFunctionExecutor<F>
where
    F: Fn(&ServerKey, I1) -> O,
    I1: NotTuple,
{
    fn execute(&mut self, input: (I1, I2)) -> O {
        let sks = self.sks.as_ref().expect("setup was not properly called");
        (self.func)(sks, input)
    }
}

Because the first case input: I1 conflicts with input: (I1, I2)

@IceTDrinker
Copy link
Member

cargo build is red and there is a conflict

@mayeul-zama mayeul-zama force-pushed the mz/strings_function_executor branch from d104914 to a185223 Compare November 25, 2024 13:20
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.

Looked good to me, @tmontaigu if you have an opinion on the generic implementation for the CPU executor stuff

Copy link
Contributor

@tmontaigu tmontaigu left a comment

Choose a reason for hiding this comment

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

I see some P: Into<PBSParameters> + Clone which surprisesme we need the clone

One minor thing that is not done on the integer tests, that slightly improves readability is to give an explicit name to the executor, e.g:

    param: P,
    mut eq_ingore_case_executor: T,
    f: for<'a> fn(&'a str, &'a str) -> bool,
) {
//...
}

impl<'a, F> FunctionExecutor<&'a mut RadixCiphertext, RadixCiphertext> for CpuFunctionExecutor<F>
/// For unary operations
///
/// Note, we need to `NotTuple` constraint to avoid conflicts with binary or ternary operations
Copy link
Contributor

Choose a reason for hiding this comment

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

impl<F, I1, O> FunctionExecutor<I1, O> for CpuFunctionExecutor<F>
where
    F: Fn(&ServerKey, I1) -> O,
    I1: NotTuple,
{
    fn execute(&mut self, input: I1) -> O {
        let sks = self.sks.as_ref().expect("setup was not properly called");
        (self.func)(sks, input)
    }
}

conflicts with

impl<F, I1, I2 O> FunctionExecutor<I1, O> for CpuFunctionExecutor<F>
where
    F: Fn(&ServerKey, I1) -> O,
    I1: NotTuple,
{
    fn execute(&mut self, input: (I1, I2)) -> O {
        let sks = self.sks.as_ref().expect("setup was not properly called");
        (self.func)(sks, input)
    }
}

Because the first case input: I1 conflicts with input: (I1, I2)

tfhe/src/strings/test_functions/test_common.rs Outdated Show resolved Hide resolved
fn setup(&mut self, _cks: &RadixClientKey, sks: Arc<ServerKey>) {
self.sks = Some(sks);
}
pub(crate) trait NotTuple {}
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@mayeul-zama mayeul-zama force-pushed the mz/strings_function_executor branch from a185223 to 459520a Compare November 26, 2024 12:31
@mayeul-zama
Copy link
Contributor Author

One minor thing that is not done on the integer tests, that slightly improves readability is to give an explicit name to the executor, e.g:

    param: P,
    mut eq_ingore_case_executor: T,
    f: for<'a> fn(&'a str, &'a str) -> bool,
) {
//...
}

Renamed executors and f as clear_function

@mayeul-zama mayeul-zama force-pushed the mz/strings_function_executor branch 2 times, most recently from b3d07ca to c525180 Compare November 26, 2024 14:59
@cla-bot cla-bot bot added the cla-signed label Nov 26, 2024
@mayeul-zama mayeul-zama force-pushed the mz/strings_function_executor branch from f9e7efa to 9fe1dc5 Compare December 2, 2024 13:24
@mayeul-zama mayeul-zama requested a review from tmontaigu December 2, 2024 13:24
@mayeul-zama mayeul-zama force-pushed the mz/strings_function_executor branch from 9fe1dc5 to 1dc8d94 Compare December 2, 2024 13:43
@mayeul-zama mayeul-zama force-pushed the mz/strings_function_executor branch from 1dc8d94 to a65fb98 Compare December 4, 2024 08:47
@zama-bot zama-bot removed the approved label Dec 4, 2024
@mayeul-zama mayeul-zama requested a review from tmontaigu December 4, 2024 08:47
@mayeul-zama mayeul-zama merged commit fd31694 into main Dec 4, 2024
99 of 106 checks passed
@mayeul-zama mayeul-zama deleted the mz/strings_function_executor branch December 4, 2024 14:05
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.

4 participants