Skip to content
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

fishy-looking math in AutoDiffScalar atan() #14051

Closed
rpoyner-tri opened this issue Sep 11, 2020 · 5 comments · Fixed by #16075
Closed

fishy-looking math in AutoDiffScalar atan() #14051

rpoyner-tri opened this issue Sep 11, 2020 · 5 comments · Fixed by #16075

Comments

@rpoyner-tri
Copy link
Contributor

Questions were raised in the context of #14045 about the details of the math implementing atan() for AutoDiffXd. I couldn't answer without a bit of history, and some way to evaluate the alternatives. This issue is to track that follow-up.

Comments:
#14045 (review)
and
#14045 (review)

@sherm1
Copy link
Member

sherm1 commented Sep 11, 2020

Assigned to @mitiguy for a look. Can you tell if the Eigen atan/asin/etc derivatives are correct for all scalar types? The implementations seem inconsistent.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Sep 11, 2020

The file in that review is only used with Scalar = double. Was the code suspicious when only used with doubles?

@sherm1
Copy link
Member

sherm1 commented Sep 11, 2020

The file in that review is only used with Scalar = double. Was the code suspicious when only used with doubles?

No, I think it is just inconsistently implemented in that case.

@jwnimmer-tri
Copy link
Collaborator

So @rpoyner-tri how about you change atan to use abs2 for consistency, and then call it a day? Or else decide we don't care and close the issue. Either way, this is not worth lingering around.

@jwnimmer-tri jwnimmer-tri assigned rpoyner-tri and unassigned mitiguy Nov 11, 2021
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Nov 11, 2021
Closes RobotLocomotion#14051.

This is mostly to answer complaints about inconsistencies with similar
functions.
@sherm1
Copy link
Member

sherm1 commented Nov 11, 2021

Per discussion in #16075 I believe the implementation with abs2() is incorrect, though the error would only be apparent for complex numbers. Other uses of abs2() there also seem wrong to me. If we want consistency, we should fix the ones that use abs2() to use plain old square instead of absolute square.

rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Nov 11, 2021
Closes RobotLocomotion#14051.

This is mostly to answer complaints about inconsistencies with similar
functions. Also, it answers complaints about confusion over the use of
abs2().
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Nov 11, 2021
Closes RobotLocomotion#14051.

This is mostly to answer complaints about inconsistencies with similar
functions. Also, it answers complaints about confusion over the use of
abs2().
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Nov 11, 2021
Closes RobotLocomotion#14051.

This is mostly to answer complaints about inconsistencies with similar
functions. Also, it answers complaints about confusion over the use of
abs2().
sherm1 pushed a commit that referenced this issue Nov 11, 2021
* autodiffxd atan(): Make consistent with other implementations

Closes #14051.

This is mostly to answer complaints about inconsistencies with similar
functions. Also, it answers complaints about confusion over the use of
abs2().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants