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

chore: update sumpool tensor operation docs #663

Merged
merged 2 commits into from
Dec 17, 2023

Conversation

field-worker
Copy link
Contributor

The flag was missing from arg description. Also added a doctest that shows the usage of the flag.
Just another small PR as I was going through and understanding the codebase.

@field-worker
Copy link
Contributor Author

As a sidenote, I noticed that some tensor ops are generic over T (e.g. pack) while others are tied to specific types, in this case i128. Do you think there'd be any value in making sumpool and others generic on T? (though I'm not mistaken, the trait bounds would have to be a little more restrictive than T: TensorType + std::marker::Send + std::marker::Sync to allow for multiplication, for example.

@alexander-camuto alexander-camuto changed the title Update sumpool tensor operation docs chore: update sumpool tensor operation docs Dec 16, 2023
@alexander-camuto
Copy link
Collaborator

alexander-camuto commented Dec 16, 2023

As a sidenote, I noticed that some tensor ops are generic over T (e.g. pack) while others are tied to specific types, in this case i128. Do you think there'd be any value in making sumpool and others generic on T? (though I'm not mistaken, the trait bounds would have to be a little more restrictive than T: TensorType + std::marker::Send + std::marker::Sync to allow for multiplication, for example.

Hey thanks for the PR. We could make it more generic -- but it would require Div and Mul if I'm not mistaken. and then it stops applying to field elements -- hence why some ops are over integers -- so as to let the end user know exactly what type works here (else a bit of a pain to debug unfortunately).

@field-worker
Copy link
Contributor Author

That's right, also pub fn const_div would need to become generic on T. I might tackle this in a follow-up

@alexander-camuto
Copy link
Collaborator

alexander-camuto commented Dec 16, 2023

That's right, also pub fn const_div would need to become generic on T. I might tackle this in a follow-up

just as a note I edited the above with

then it stops applying to field elements -- hence why some ops are over integers -- so as to let the end user know exactly what type works here (else a bit of a pain to debug unfortunately).

Down to make it more generic though :)

@field-worker
Copy link
Contributor Author

I'm not sure I follow - you want to be able to have a Tensor over field elements, or just over primitive types?
If the former, AFAIK Field already implements Mul. While Div trait is not directly specified in the bounds, in theory it's possible to divide two prime field elements.

But now that I look at it, I think Div won't be needed after all: https://github.com/field-worker/ezkl/blob/b1582bde89df7b87f52d374d8930a5dc1a6abec5/src/tensor/ops.rs#L3731

@alexander-camuto
Copy link
Collaborator

DM me on twitter so we can discuss in full (@CamutoDante)

@field-worker
Copy link
Contributor Author

Happy to, but you'd have to msg me or follow back: https://twitter.com/field_worker99

@alexander-camuto alexander-camuto merged commit 5ddb1c6 into zkonduit:main Dec 17, 2023
13 checks passed
@field-worker field-worker deleted the update-sumpool-docs branch December 17, 2023 19:00
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

Successfully merging this pull request may close these issues.

2 participants