-
Notifications
You must be signed in to change notification settings - Fork 8
Add rules for atan2, hypot, mod, and rem. Addresses #15. #16
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
Conversation
src/rules.jl
Outdated
| @define_diffrule atan2(x, y) = :($y / ($x*$x + $y*$y)), :(-$x / ($x*$x + $y*$y)) | ||
| @define_diffrule hypot(x, y) = :($x / hypot($x, $y)), :($y / hypot($x, $y)) | ||
| @define_diffrule mod(x, y) = :(1), :(-trunc($x / $y)) | ||
| @define_diffrule rem(x, y) = :(1), :(-trunc($x / $y)) |
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.
These need fixing. mod and rem are not identical. Besides, contrary to my belief following an initial quick study of the cases, these rules don't work for all sign combinations for either function.
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.
Nope. I was wrong about being wrong. 😄 The rule works for rem, but trunc needs to be changed to floor for mod.
|
The rules for Should they be modified to give |
I think so, might as well have them be correct. We could change the rule to something like Thanks for this! |
|
Oh, forgot to do the same for the first partial. |
|
I'll squash shortly. |
src/rules.jl
Outdated
| # mod | ||
| # hypot | ||
| # atan2 | ||
| @define_diffrule atan2(x, y) = :($y / ($x*$x + $y*$y)), :(-$x / ($x*$x + $y*$y)) |
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.
Can we switch x * x and y * y to x^2 and y^2, just to keep in style with the other rules? Thanks!
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.
K
59b55ba to
6220c2f
Compare
Addresses #15.