-
Notifications
You must be signed in to change notification settings - Fork 130
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
feat: add halo2_debug package #346
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
==========================================
+ Coverage 81.90% 82.28% +0.38%
==========================================
Files 82 83 +1
Lines 17019 17458 +439
==========================================
+ Hits 13939 14366 +427
- Misses 3080 3092 +12 ☔ View full report in Codecov by Sentry. |
} | ||
} | ||
|
||
/// Wrapper type over `Expression` that implements Display with nice 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.
Why adding a wrapper when we own the Expression
type and we code in in an impl
?
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.
The reason for the wrapper is to be able to include auxiliary information that allows displaying the expressions with column names.
In the Expression
type we don't hold the column names, just the column type and index. Then in ConstraintSystemMid
we have a HashMap
for column names.
ExprDisp
can have a reference to this HashMap
so that we can show the columns by name, like in this example
halo2/halo2_proofs/tests/compress_selectors.rs
Lines 465 to 468 in 458ad70
assert_eq!( | |
"s_add * (l + r - o)", | |
format!("{}", expr_disp_names(&cs.gates[0].poly, names)) | |
); |
Without these extra data, we could only print generic names like advice0, fixed2, instance3
. I think being able to use named columns makes the expression much more readable.
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.
Love this new feature! LGTM 👍
|
||
impl<F: PrimeField> fmt::Display for FDisp<'_, F> { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
let v = (*self.0).to_repr(); |
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 have this be a bit cleaner if we just had access to F::MODULUS
... For now, I think this is as good as it can be done!
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.
Thanks for making the human life easier.
LGTM
Introduce a
halo2_debug
package that fills the need identified in #343For now it only contains helper types to nicely display prime fields, expressions, lookups and shuffles.
As an example usage I extended the
compress_selectors
tests:halo2/halo2_proofs/tests/compress_selectors.rs
Lines 465 to 476 in 458ad70
NOTE: I just saw this #345 we'll have to converge into the same name 😄