Skip to content

Conversation

fboemer
Copy link
Contributor

@fboemer fboemer commented May 2, 2025

Make HeContext protocol, which different HE schemes can implement.

@fboemer fboemer force-pushed the fboemer/flexible-context-for-he-scheme branch 4 times, most recently from 5a71a0a to 28c8e83 Compare May 2, 2025 16:38
@fboemer fboemer changed the title Fboemer/flexible context for he scheme Decouple Context and Scheme and make context for HeScheme May 2, 2025
@fboemer fboemer changed the title Decouple Context and Scheme and make context for HeScheme Decouple Context and Scheme and make HeScheme context flexible May 2, 2025
@fboemer fboemer added the ⚠️ semver/major Breaks existing public API label May 2, 2025
@fboemer fboemer marked this pull request as ready for review May 2, 2025 16:51
@fboemer fboemer requested review from karulont and RuiyuZhu as code owners May 2, 2025 16:51
@karulont
Copy link
Contributor

karulont commented May 5, 2025

Introduction of HeContext is good.
But I see no big value in removing HeScheme from Context.
Context<Scheme: HeScheme> is in my opinion better than Context<Scalar> and then passing in Scheme separately.

@fboemer fboemer marked this pull request as draft May 6, 2025 16:56
@fboemer
Copy link
Contributor Author

fboemer commented May 6, 2025

Introduction of HeContext is good. But I see no big value in removing HeScheme from Context. Context<Scheme: HeScheme> is in my opinion better than Context<Scalar> and then passing in Scheme separately.

SGTM; faa9a32 has the delta.

@fboemer fboemer force-pushed the fboemer/flexible-context-for-he-scheme branch 2 times, most recently from bf26507 to 5ac4035 Compare May 6, 2025 18:08
@fboemer fboemer marked this pull request as ready for review May 6, 2025 20:25
@fboemer fboemer force-pushed the fboemer/flexible-context-for-he-scheme branch from 5ac4035 to 2cc800e Compare May 6, 2025 20:29
@fboemer fboemer changed the title Decouple Context and Scheme and make HeScheme context flexible Make HeScheme context flexible May 6, 2025
Copy link
Contributor

@karulont karulont left a comment

Choose a reason for hiding this comment

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

Removing Scheme from Context required a lot of extra type annotations. Now that we put the scheme back into context. Those type annotations are not needed, and I would rather like to keep the change small and revert unnecessary changes.

@fboemer fboemer marked this pull request as draft May 7, 2025 18:41
@fboemer fboemer self-assigned this May 8, 2025
@fboemer fboemer marked this pull request as ready for review May 8, 2025 21:00
@fboemer fboemer marked this pull request as draft May 8, 2025 21:00
@fboemer
Copy link
Contributor Author

fboemer commented May 8, 2025

This works:

    public static func multiplyInverseTest<Context: HeContext>(context: Context) async throws where Context.Scheme.Context == Context {

Note, the more natural

    public static func multiplyInverseTest<Scheme: HeScheme>(context: Scheme.Context) async throws {

fails due to Generic parameter Scheme is not used in function signature.

We previously had

    public static func multiplyInverseTest<Scheme: HeScheme>(context: Context<Scheme>) async throws {

but that constrains use of the function to schemes for which Scheme.Context == Context<Scheme>

@fboemer fboemer force-pushed the fboemer/flexible-context-for-he-scheme branch 2 times, most recently from 7e9e7a4 to 9a0cfaa Compare May 9, 2025 14:07
@fboemer fboemer marked this pull request as ready for review May 9, 2025 14:42
@fboemer fboemer requested a review from karulont May 9, 2025 14:42
@fboemer fboemer force-pushed the fboemer/flexible-context-for-he-scheme branch from 9a0cfaa to 0d428eb Compare May 9, 2025 14:43
@fboemer fboemer force-pushed the fboemer/flexible-context-for-he-scheme branch 2 times, most recently from c61891b to b13680b Compare May 9, 2025 22:55
@fboemer fboemer force-pushed the fboemer/flexible-context-for-he-scheme branch from b13680b to 99b939e Compare May 10, 2025 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ semver/major Breaks existing public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants