-
Notifications
You must be signed in to change notification settings - Fork 40
BaseFold: Add and reimplement some utility functions. #559
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
Conversation
|
Thanks for breaking down the big PR into more digestible chunks! |
matthiasgoergens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add in the PR description why we are making this refactoring?
Just a quick note about what task becomes easier or simpler with the new organisation?
Thanks!
| ) -> impl Iterator<Item = &E::BaseField> + '_ { | ||
| match values { | ||
| FieldType::Ext(coeffs) => Either::Left(coeffs.iter().flat_map(|x| x.as_bases())), | ||
| FieldType::Base(coeffs) => Either::Right(coeffs.iter()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the Either::Left and Either::Right here is nessesary? Because return type are the same, here somehow Either is a bit over design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the return types are not the same. They are both iterators over BaseField but different types of Iterator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the standard solution for this? The Either is some solution I found on the internet after many fruitless fightings with the compiler. Any better suggestions are welcome. I'm really not an expert in rust iterators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is rely on runtime dispatch.
diff --git a/mpcs/src/util.rs b/mpcs/src/util.rs
index 7688b53e..6edd82d4 100644
--- a/mpcs/src/util.rs
+++ b/mpcs/src/util.rs
@@ -182,10 +182,10 @@ pub fn field_type_as_ext<E: ExtensionField>(values: &FieldType<E>) -> &Vec<E> {
pub fn field_type_iter_base<E: ExtensionField>(
values: &FieldType<E>,
-) -> impl Iterator<Item = &E::BaseField> + '_ {
+) -> Box<dyn Iterator<Item = &E::BaseField> + '_> {
match values {
- FieldType::Ext(coeffs) => Either::Left(coeffs.iter().flat_map(|x| x.as_bases())),
- FieldType::Base(coeffs) => Either::Right(coeffs.iter()),
+ FieldType::Ext(coeffs) => Box::new(coeffs.iter().flat_map(|x| x.as_bases())),
+ FieldType::Base(coeffs) => Box::new(coeffs.iter()),
_ => unreachable!(),
}
}After reading document in Either, I think static dispatch via Either is better, and either already implement iterator so it's truly works this purpose. So LGTM under this scope! 👍
hero78119
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Extracting small PRs from #294
There was an implementation of Iterator for iterating through a
FieldType. Turns out that it can be replaced with existing tools initer_tools. This proposes the new implementation.Also added some other utility functions for type conversion between
FieldTypeandVec<E>, to avoidmatch field_typeeverywhere.