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

Editorial: fix algorithm of Math.atan2 #3170

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Conversation

Josh-Cena
Copy link
Contributor

Fix #2897.

I have no idea if this meets the conventions, but I hope the idea is at least correct. Below is the output of Math.atan2 in Node:

image

@Josh-Cena Josh-Cena force-pushed the fix-atan2 branch 2 times, most recently from a99ee7e to 231b8c7 Compare September 10, 2023 17:56
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb added editorial change editor call to be discussed in the next editor call labels Dec 6, 2023
@bakkot bakkot added the es2024 to be landed before es2024 label Jan 10, 2024
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jan 11, 2024
@bakkot
Copy link
Contributor

bakkot commented Jan 24, 2024

Thanks for the PR, but isn't this wrong? Consider the case in your issue: y=1, x=-1. In that case we'll have r = atan(1/-1) i.e. r = -0.7853981633974483. Then it will fall into the nx < 0, ny > 0 case, which will do r = π - r, i.e. 3.9269908169872414. That's wrong; it should be 2.356194490192345.

I think this ends up simplest if we do the inverse tan of the absolute value of y/x, and then specify the quadrant explicitly. Basically

function atan2(y, x) {
  let r = Math.atan(Math.abs(y/x));
  if (x > 0 && y > 0) return r;
  if (x > 0 && y < 0) return -r;
  if (x < 0 && y > 0) return Math.PI - r;
  if (x < 0 && y < 0) return -Math.PI + r;
}

@Josh-Cena want to update the PR to work like that? Otherwise I'll get to it in the next week or two.

@bakkot bakkot added the editor call to be discussed in the next editor call label Jan 24, 2024
@Josh-Cena
Copy link
Contributor Author

OK, I'll take a look. If I don't get to it in the next week or two then I probably will not ever :P

@Josh-Cena
Copy link
Contributor Author

@bakkot I realized I only need to take abs for x (by referencing the diagram above). This way I avoid the extra negation when x > 0 && y < 0.

@bakkot
Copy link
Contributor

bakkot commented Jan 25, 2024

@Josh-Cena That does work but I think it's significantly harder to understand than the version which just lists all four cases explicitly. Avoiding an extra negation in the spec algorithms isn't really a goal.

@Josh-Cena Josh-Cena force-pushed the fix-atan2 branch 2 times, most recently from 9a291d2 to 648f25f Compare January 25, 2024 00:41
spec.html Outdated Show resolved Hide resolved
@michaelficarra michaelficarra added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Feb 3, 2024
@ljharb ljharb merged commit ddb41dd into tc39:main Feb 5, 2024
7 checks passed
@Josh-Cena Josh-Cena deleted the fix-atan2 branch February 5, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change es2024 to be landed before es2024 ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Math.atan2() should not return "inverse tangent of ny / nx"
5 participants