-
Notifications
You must be signed in to change notification settings - Fork 78
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
Recursive Redshift #20
base: master
Are you sure you want to change the base?
Conversation
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.
A lot of:
- TODOs
- unannotated asserts
- asserts themselves (probably panics should be replaced with
Result
s somewhere); at least, annotations explaining why a panic is safe would be helpful - commented out code
- wrong comment types:
//
instead of///
and///
instead of//!
- can be easily fixed with auto-replace to simplify docs generating in the future
Also, do not forget to run cargo fmt
and cargo clippy
before commiting.
let geninv = self.geninv; | ||
self.distribute_powers(worker, geninv); | ||
|
||
// worker.scope(self.coeffs.len(), |scope, chunk| { |
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.
Please, annotate why it is commented out.
/// however the values stored in FRI oracles are permuted, so that elements of the same coset are adjacent | ||
/// So, we need to somehow get all elements of rhe same coset using natural index [i], i.e. | ||
/// make a conversion from "natural" element index to "tree" coset index | ||
/// note, that this depends only on the number of elements in each coset i.e. "collapsing factor" |
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.
Please, change to //!
module docs.
https://doc.rust-lang.org/1.9.0/book/documentation.html#documenting-modules
|
||
|
||
impl CosetCombiner { | ||
//wrapping factor here is size of coset: 1 << collapsing_factor |
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.
Please, change to doc comment.
/// wrapping factor here is size of coset: 1 << collapsing_factor
pub final_degree_plus_one : usize, | ||
} | ||
|
||
//TODO: paranetrize FriIop with coset combiner also |
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.
Please, pay attention.
use crate::redshift::fft::cooley_tukey_ntt::bitreverse; | ||
use std::ops::Range; | ||
|
||
//TODO: it is also very important to understand how do values are located inside coset |
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.
Please, pay attention.
// self.coeffs.resize(new_size, F::zero()); | ||
|
||
// Ok(()) | ||
// } |
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.
Please, annotate why is it commented out.
let n = input_gates.len() + aux_gates.len(); | ||
|
||
//check consistency of n and FRI-parameters | ||
// TODO: I may be mistaken here and should have simply n instead of n+1 - CHECK THIS! |
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.
Please, pay attention.
let w_r = Polynomial::<E::Fr, Values>::from_values_unpadded(w_r)?; | ||
let w_o = Polynomial::<E::Fr, Values>::from_values_unpadded(w_o)?; | ||
|
||
// TODO: replace by ifft_using_bitreversed_ntt_with_partial_reduction |
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.
Please, pay attention.
assert!(z_2.as_ref().last().expect("must exist") == z_1.as_ref().last().expect("must exist")); | ||
|
||
// interpolate on the main domain | ||
// TODO: replace by ifft_using_bitreversed_ntt_with_partial_reduction |
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.
Please, pay attention.
// mat = matrix([[z^(i*j) for j in range(0, 2*m)] for i in range(0, m)]) | ||
// return mat.echelon_form()[:, m:] | ||
pub(crate) fn generate_mds_matrix<F: PrimeField>(_params: &RescueParams<F>) -> [[F; RESCUE_M]; RESCUE_M] { | ||
// TODO: Correct MDS generation; this causes horribly-biased output |
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.
Please, pay attention.
Do not merge!