-
Notifications
You must be signed in to change notification settings - Fork 86
Update Math Demo #757
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
base: main
Are you sure you want to change the base?
Update Math Demo #757
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.
Pull request overview
This PR updates the math demo to closely match the egg implementation from https://github.com/egraphs-good/egg/blob/main/tests/math.rs. The changes improve performance and align the implementation with the reference design.
- Replaces
i64withf64for floating-point constants throughout - Implements node pruning using subsumption in a separate
pruneruleset - Simplifies constant handling by replacing the
evals-torelation with direct constant folding rewrites - Replaces the
is-symrelation with inline Var pattern matching - Adds comprehensive test cases from egg covering differentiation, integration, and simplification
- Updates cost model to match egg (adds
:cost 100to Diff and Integral)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (relation is-not-zero (Math)) | ||
| (rule ((evals-to x vx) | ||
| (!= vx 0)) | ||
| ; Is not zero is set as anything besides the constant 0.0 | ||
| (rule ((MathU x) | ||
| (!= x (Const 0.0))) | ||
| ((is-not-zero x))) |
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.
Avoid tagging all non‑zero literals as proven non‑zero
The new is-not-zero rule now asserts is-not-zero x for every MathU term except the literal (Const 0.0). That makes conditional rewrites such as (Div a a) -> (Const 1.0) and (Pow x (Const 0.0)) -> (Const 1.0) fire for any symbolic variable, so Div (Var "x") (Var "x") now rewrites to 1 even when x could be zero. The previous evals-to-based predicate only marked values proven non‑zero; this broad rule introduces unsound equalities for zero inputs.
Useful? React with 👍 / 👎.
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.
FWIW I updated the rule to be this because the egg rule is also unsound like this and that behavior is actually tested
CodSpeed Performance ReportMerging #757 will not alter performanceComparing Summary
Footnotes
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Maybe we want to keep both? I thought the other math file was meant to be more natural egglog demonstrating evals-to |
|
Sounds good, moved it to a new file |
yihozhang
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.
Maybe we should just keep one in the web-demo and move the other one out?
This PR updates the math demo to more closely resemble the one from egglog at https://github.com/egraphs-good/egg/blob/main/tests/math.rs:
evals-toand replaces with just directly unioning the constis-symand replaces by just inlining Var patternThis also runs quite quickly now on my machine!