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

autodiffxd atan(): Use abs2() #16075

Merged
merged 1 commit into from
Nov 11, 2021
Merged

Conversation

rpoyner-tri
Copy link
Contributor

@rpoyner-tri rpoyner-tri commented Nov 11, 2021

Closes #14051.

This is mostly to answer complaints about inconsistencies with similar
functions.


This change is Reviewable

@rpoyner-tri rpoyner-tri added the status: single reviewer ok https://drake.mit.edu/reviewable.html label Nov 11, 2021
Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+(status: single reviewer ok)
+@sherm1 for both reviews.

Reviewable status: LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @sherm1)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @rpoyner-tri and @sherm1)


common/autodiffxd.h, line 461 at r1 (raw file):

DRAKE_EIGEN_AUTODIFFXD_DECLARE_GLOBAL_UNARY(
    atan, using std::atan;
    x.derivatives() *= Scalar(1) / (1 + numext::abs2(x.value()));

This seems wrong to me. According to all the references I could find, the derivative of atan(z) is 1/(1 + z²). With the proposed change, this
would be 1/(1 + |z|²). When z is real, those are the same.
But when z=a+bi is complex
|z|²=a²+b² while z²= a²−b² + 2abi. The former is the "absolute square" (product of z and conj(z)) and the latter is just the "square" (product of z with itself).

I believe all the other uses of abs2 here are also wrong. Since we don't support complex in Drake I suppose it doesn't matter but I wouldn't compound the problem by making this one wrong also!

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @rpoyner-tri and @sherm1)


common/autodiffxd.h, line 461 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

This seems wrong to me. According to all the references I could find, the derivative of atan(z) is 1/(1 + z²). With the proposed change, this
would be 1/(1 + |z|²). When z is real, those are the same.
But when z=a+bi is complex
|z|²=a²+b² while z²= a²−b² + 2abi. The former is the "absolute square" (product of z and conj(z)) and the latter is just the "square" (product of z with itself).

I believe all the other uses of abs2 here are also wrong. Since we don't support complex in Drake I suppose it doesn't matter but I wouldn't compound the problem by making this one wrong also!

Let me anticipate the follow-up question we will get from the peanut gallery.

"You have expression * expression here, isn't that going to evaluate expression twice, at some cost?"

Maybe yes, maybe no, depending on optimizer details. I doubt we care enough to write the code to measure alternatives.

@jwnimmer-tri
Copy link
Collaborator


common/autodiffxd.h, line 461 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Let me anticipate the follow-up question we will get from the peanut gallery.

"You have expression * expression here, isn't that going to evaluate expression twice, at some cost?"

Maybe yes, maybe no, depending on optimizer details. I doubt we care enough to write the code to measure alternatives.

As I said in the original PR -- Scalar is always double here. Imaginary doesn't matter.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @rpoyner-tri and @sherm1)


common/autodiffxd.h, line 461 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

As I said in the original PR -- Scalar is always double here. Imaginary doesn't matter.

@rpoyner-tri: x.value() is just an accessor, not a computation. So I don't think there is an efficiency argument.

@jwnimmer-tri: writing the math correctly in the source code does matter. If we wanted to use the wrong expression to take computational advantage of our knowledge of the particular scalar type that would be reasonable if surrounded by explanatory comments to avoid future cargo culting or even just looking like we don't know what we're doing. However, in this case there is no computational advantage and no reason to use anything other than x*x here.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @rpoyner-tri and @sherm1)


common/autodiffxd.h, line 461 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

@rpoyner-tri: x.value() is just an accessor, not a computation. So I don't think there is an efficiency argument.

@jwnimmer-tri: writing the math correctly in the source code does matter. If we wanted to use the wrong expression to take computational advantage of our knowledge of the particular scalar type that would be reasonable if surrounded by explanatory comments to avoid future cargo culting or even just looking like we don't know what we're doing. However, in this case there is no computational advantage and no reason to use anything other than x*x here.

Whatever. This entire issue is a waste of everyone's time. And so, I should just take my own advice and unsubscribe from this entire review and issue -- it's certainly not worth my time. I'm marking myself resolved here.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @rpoyner-tri and @sherm1)


common/autodiffxd.h, line 461 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Whatever. This entire issue is a waste of everyone's time. And so, I should just take my own advice and unsubscribe from this entire review and issue -- it's certainly not worth my time. I'm marking myself resolved here.

I would also be happy with just closing the original issue and taking no action. I'm only objecting to making it worse.

If you do want to make a change here I think the best one would be to remove the TODO here and move it to the fishy uses of abs2() elsewhere. To actually "fix" the code, we would have to check carefully all the derivatives to make sure there isn't one that actually needs abs2(). But nothing would actually change since we're specialized on double so this might not be the best use of time!

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @rpoyner-tri and @sherm1)


common/autodiffxd.h, line 461 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

I would also be happy with just closing the original issue and taking no action. I'm only objecting to making it worse.

If you do want to make a change here I think the best one would be to remove the TODO here and move it to the fishy uses of abs2() elsewhere. To actually "fix" the code, we would have to check carefully all the derivatives to make sure there isn't one that actually needs abs2(). But nothing would actually change since we're specialized on double so this might not be the best use of time!

cosh(x.value()) is not just an accessor. Also, I don't think there is an efficiency argument, but changes like this attract bikeshedding like no other.

Copy link
Contributor Author

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @sherm1)


common/autodiffxd.h, line 461 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

cosh(x.value()) is not just an accessor. Also, I don't think there is an efficiency argument, but changes like this attract bikeshedding like no other.

Done.

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().
Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should silence the haters! Thanks.
:lgtm: x 2

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: missing label for release notes (waiting on @rpoyner-tri)

@sherm1 sherm1 added release notes: none This pull request should not be mentioned in the release notes type: cleanup labels Nov 11, 2021
@sherm1 sherm1 merged commit 92bdeee into RobotLocomotion:master Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes status: single reviewer ok https://drake.mit.edu/reviewable.html type: cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fishy-looking math in AutoDiffScalar atan()
3 participants