fix: handle full_output compression correctly
#461
Conversation
whichqua
changed the title
full_output compression correctly
|
This looks REALLY good 🚀 The check orphan job is failing cause is using the old script. That will be fixed once this is merge in the other branch 💪 |
There was a problem hiding this comment.
Looking great in general @whichqua
As this is a very complex hint I've asked for much more doc and unit tests
| static ref HEADER_ELM_BOUND: BigUint = BigUint::one() << HEADER_ELM_N_BITS; | ||
| } | ||
|
|
||
| #[derive(Default, Clone, Debug)] |
There was a problem hiding this comment.
Add docs explaining what this struct is needed for, what is the data it contains, etc
| } | ||
|
|
||
| pub const COMPRESS: &str = indoc! {r#"from starkware.starknet.core.os.data_availability.compression import compress | ||
| fn get_n_elms_per_felt(elm_bound: &BigUint) -> usize { |
There was a problem hiding this comment.
Add docs explaining what this function is doing
| } | ||
| } | ||
|
|
||
| #[derive(Default, Clone, Debug)] |
There was a problem hiding this comment.
Add doc on what this struct and its data is for
|
|
||
| let packed_bucket_index_per_elm = pack_in_felts(bucket_index_per_elm, TOTAL_N_BUCKETS as usize); | ||
| fn pack_in_felts(elms: &[BigUint], elm_bound: &BigUint) -> Vec<BigUint> { |
| .chain(packed_repeating_value_pointers.into_iter()) | ||
| .chain(packed_bucket_index_per_elm.into_iter()) | ||
| .collect::<Vec<Felt252>>(); | ||
| fn pack_in_felt(elms: &[BigUint], elm_bound: &BigUint) -> BigUint { |
| if value.bits() as usize <= bucket.n_bits { | ||
| if bucket.contains(value) { | ||
| self.repeating_value_locations.push((*bucket_index, bucket.get_index(value))); | ||
| self.bucket_index_per_elm.push(self.repeating_values_bucket_index()); |
There was a problem hiding this comment.
I dont follow why this is the len() of buckets and not the bucket_index
| self.bucket_index_per_elm.push(*bucket_index); | ||
| } | ||
| break; |
There was a problem hiding this comment.
This break is to store the value in the first bucket that can store it, right? (and not continue adding it to other "bigger" buckets)
| /// Returns a list of pointers corresponding to the repeating values. | ||
| /// The pointers point to the chained unique value buckets. | ||
| pub fn get_repeating_value_pointers(&self) -> Vec<usize> { | ||
| fn get_repeating_value_pointers(&self) -> Vec<BigUint> { |
There was a problem hiding this comment.
Also lets add docs and unit tests
| self.buckets.len() | ||
| } | ||
|
|
||
| fn update(&mut self, values: &[BigUint]) { |
There was a problem hiding this comment.
Lets add docs and some unit test for this function.
Its complex to follow what it should be doing.
| #[rstest] | ||
| #[case::single_value_1(vec![1u32], vec!["1393796574908163946345982392040522595172352", "1", "5"])] | ||
| #[case::single_value_2(vec![2u32], vec!["1393796574908163946345982392040522595172352", "2", "5"])] | ||
| #[case::single_value_3(vec![10u32], vec!["1393796574908163946345982392040522595172352", "10", "5"])] | ||
| #[case::two_values(vec![1u32, 2], vec!["2787593149816327892691964784081045190344704", "65537", "40"])] | ||
| #[case::three_values(vec![2u32, 3, 1], vec!["4181389724724491839037947176121567785517056", "1073840130", "285"])] | ||
| #[case::four_values(vec![1u32, 2, 3, 4], vec!["5575186299632655785383929568162090380689408", "140740709646337", "2000"])] | ||
| #[case::extracted_kzg_example(vec![1u32, 1, 6, 1991, 66, 0], vec!["1461508606313777459023416562628243222268909453312", "2324306378031105", "0", "98047"])] |
There was a problem hiding this comment.
How these test cases where calculated? Could you add the link where these test lives?
Issue Number: N/A
Type
Description
This PR provides:
BigUint.HashMapwithIndexMap.compressimplementation.compressmethod.Breaking changes?