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

KyberModue creates Matrix instead of KyberMatrix #36

Open
jpgoldberg opened this issue Jul 21, 2024 · 4 comments
Open

KyberModue creates Matrix instead of KyberMatrix #36

jpgoldberg opened this issue Jul 21, 2024 · 4 comments

Comments

@jpgoldberg
Copy link

There is some type/class confusion between Module and its subclass KyberModule in that __call__ is inherited from the from the former. As a consequence it is unclear whether a "call" to KyberModule produces a Matrix or a KyberMatrix. This also is affecting the vector methods.

There are a number of ways to sort this out, but it is hard to get type clarity until it is sorted out.

Personally, I would like to do away with __call__ in the first place, but we can also fix things through other ways of wrapping the calls.

@GiacomoPope
Copy link
Owner

I'm not sure I entirely like my design with the subclasses -- originally all parameters of Kyber were tweakable (so n and q as well as everything else) but I think for standard users this will be more confusing than helpful.

For the polynomial ring elements having Polynomial and PolynomialNTT made sense (these things having different types made sense to me at least)

I'd be happy to discuss in this issue what you think would be cleanest for the design of the Polynomial and Module classes. Happy to do the work myself guided by what you think is best too.

@GiacomoPope
Copy link
Owner

Maybe I should just make the decode vector a class method of Matrix and then delete the module class altogether

@GiacomoPope
Copy link
Owner

The hope I had was that Matrix uses call with self.matrix so the type is right, but maybe from the point of view of type checking this is annoying?

@GiacomoPope
Copy link
Owner

Can this be closed? I think everything is OK now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants