-
Notifications
You must be signed in to change notification settings - Fork 97
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
cross product utility function. #1031
Conversation
1a01778
to
0dd85cf
Compare
std/binary.asm
Outdated
col fixed P_B(i) { (i >> 8) % 256 }; | ||
col fixed P_operation(i) { (i / (256 * 256)) % 3 }; | ||
// TOOD would be nice with destructuring assignment for arrays. | ||
let inputs: (int -> int)[] = split_bits([2, 8, 8]); |
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.
let inputs: (int -> int)[] = split_bits([2, 8, 8]); | |
let input_output_pairs: (int -> int)[] = cross_product([3, 256, 256]); |
What do you think of this? Two changes:
- I'm thinking of it in terms of a cross product, so at least for me it would be a better name.
- I think the function could be rewritten to support cross products of ranges of arbitrary size. This would actually yield a different result and enforce that
0 <= op < 3
. Then the match below would actually be exhaustive, like it was before.
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.
sounds good!
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.
We could even have a type at some point that stores a function and its cycle size. I think that is useful for implementing the [....]*
-opreator as well.
e15b0b7
to
b861a84
Compare
let op = inputs[0]; | ||
col fixed P_A(i) { a(i) }; | ||
col fixed P_B(i) { b(i) }; | ||
col fixed P_operation(i) { op(i)}; |
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.
Why do we still need PA PB...?
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.
They are used in the lookup below. Also, a and b are int -> int
functions (we need that because if the binary operators) and the columns have the implicit conversion to fe.
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.
Maybe cross_product
could return an array of columns (i.e., (int -> fe)[]
)? I think it would be cleaner overall, I don't find the int
conversion below too bad.
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.
Or, another idea, cross_product()
could also receive a (int[] -> int)
to populate the result. But that assumes that there is only one output column.
|
||
let cross_product_internal: int, int[] -> (int -> int)[] = |cycle_len, len, sizes| | ||
if len == 0 { | ||
// We could assert here that the degree is at least `cycle_len` |
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.
That would be great! Can we do it in this PR, or is there something blocking it?
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.
We don't have access to the degree. I think we should do this at a later point.
let op = inputs[0]; | ||
col fixed P_A(i) { a(i) }; | ||
col fixed P_B(i) { b(i) }; | ||
col fixed P_operation(i) { op(i)}; |
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.
Maybe cross_product
could return an array of columns (i.e., (int -> fe)[]
)? I think it would be cleaner overall, I don't find the int
conversion below too bad.
let op = inputs[0]; | ||
col fixed P_A(i) { a(i) }; | ||
col fixed P_B(i) { b(i) }; | ||
col fixed P_operation(i) { op(i)}; |
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.
Or, another idea, cross_product()
could also receive a (int[] -> int)
to populate the result. But that assumes that there is only one output column.
I think the proper way to do this (but that requires structs) is to make |
The idea with the output function is good, but I think we need destructuring assignments first to make it nice. |
No description provided.