-
Notifications
You must be signed in to change notification settings - Fork 42
Add fabs support to Taylor series expander #1275
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?
Conversation
I can confirm that this fixes #1271, at least on my machine. |
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.
I slightly misunderstand how the code works, but it looks fine to me
The biggest drop is "a from scale-rotated-ellipse" but it's kind of impossible to tell what's going on there. |
Looking a bit more I realize that this branch wasn't up to date with |
Ok, I looked at a couple of cases and they are all cases of over-greediness. Basically, the ability to Taylor-expand more stuff means that Herbie achieves better results after iter 1, but that only prevents it from searching harder for a rewrite-based solution. |
This PR adds support for
fabs
in the Taylor expander. It thus fixes #1271. Specifically,(fabs (+ x 1))
expands to(+ 1 x)
;(fabs (- x 1))
expands to(+ 1 (neg x))
(it gets negated because the constant term is positive); and finally, terms like(fabs (+ x (* x x))
expands to(+ (fabs x) (* (fabs x) x))
which is confusing but accurate in a neighborhood near 0.Initially I had copilot write it and I was impressed that it came up with something plausible, but actually its code didn't work at all, so I had to rewrite from scratch basically. https://chatgpt.com/codex/tasks/task_e_684b1775fec4833193db8dcc2a906e7a
This PR also includes a tiny amount of cleanup for trigonometric stuff, leveraging new helper function I added for
fabs
.