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

Jump to bottom

math: discrepancy in Atan2 special case #35446

Closed
bmkessler opened this issue Nov 8, 2019 · 7 comments
Closed

math: discrepancy in Atan2 special case #35446

bmkessler opened this issue Nov 8, 2019 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@bmkessler
Copy link
Contributor 

$ go version
go version go1.13.4 linux/amd64

Does this issue reproduce with the latest release?

yes

The special cases for Atan2 are

// Special cases are (in order):
//	Atan2(y, NaN) = NaN
//	Atan2(NaN, x) = NaN
//	Atan2(+0, x>=0) = +0
//	Atan2(-0, x>=0) = -0
//	Atan2(+0, x<=-0) = +Pi
//	Atan2(-0, x<=-0) = -Pi
//	Atan2(y>0, 0) = +Pi/2
//	Atan2(y<0, 0) = -Pi/2
//	Atan2(+Inf, +Inf) = +Pi/4
//	Atan2(-Inf, +Inf) = -Pi/4
//	Atan2(+Inf, -Inf) = 3Pi/4
//	Atan2(-Inf, -Inf) = -3Pi/4
//	Atan2(y, +Inf) = 0
//	Atan2(y>0, -Inf) = +Pi
//	Atan2(y<0, -Inf) = -Pi
//	Atan2(+Inf, x) = +Pi/2
//	Atan2(-Inf, x) = -Pi/2

Atan2(y, +Inf) = 0 is handled differently between the pure go code and the s390x implementation.

The pure go implementation actually conforms to C99 Annex F. 9.1.4 which specifies:

atan2(±y,+∞) returns ± 0 for finite y>0.

Specifically, the sign of zero depends on the sign of y, while the s390x implementation returns +0 for all values of y.

Note that this behavior affects special case handling of math/cmplx as well since those functions rely on math for the underlying implementations.

A decision should be made on what the specification should state for the sign of zero in this case. I am not sure if updating the go specification to depend on the sign of y (C99) would be incompatible with the go compatibility promise since the current specification doesn't explicitly state that the sign should be positive. It seems that the majority implementation is already conforming to the C99 specification in any case.
@bmkessler
Copy link
Contributor Author

@griesemer @ianlancetaylor @mundaym

@bmkessler
Copy link
Contributor Author

This causes s390x test failures on #29320 for cmplx.Log and cmplx.Log10

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 8, 2019
@bcmills bcmills added this to the Backlog milestone Nov 8, 2019
@bcmills
Copy link
Contributor

bcmills commented Nov 8, 2019

I am not sure if updating the go specification to depend on the sign of y (C99) would be incompatible with the go compatibility promise since the current specification doesn't explicitly state that the sign should be positive.

In general the math functions aim for consistency with their C equivalents, I believe. And certainly the Go compatibility policy allows the implementation to be made consistent across platforms.

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 8, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 8, 2019
@mundaym
Copy link
Member

mundaym commented Nov 8, 2019

I'm happy to add the Atan2(±y, +Inf) = ±0 special case to the s390x implementation if that helps. I don't think we necessarily need to update the documentation for Atan2 but it would be good to add a test to enforce this special case even if we don't guarantee it externally.

@gopherbot
Copy link

Change https://golang.org/cl/220689 mentions this issue: math/cmplx: handle special cases

@gopherbot
Copy link

Change https://golang.org/cl/223420 mentions this issue: math: correct Atan2(±y,+∞) = ±0 on s390x

@bmkessler
Copy link
Contributor Author

Note that there are tests already in place to handle this special case correctly.

https://github.com/golang/go/blob/master/src/math/all_test.go#L848-L866

	Copysign(0, -1), // atan2(-Pi, Inf)
	Copysign(0, -1), // atan2(-0, +Inf)
	0,               // atan2(+0, +Inf)
	0,               // atan2(+Pi, +Inf)

But the s390x assembly just has a special case specifically to pass the (-Pi, +Inf) test case.

https://github.com/golang/go/blob/master/src/math/atan2_s390x.s#L137

	//special case Atan2(-Pi, +Inf) = Pi
	MOVD	$NegPi, R3
	CMPUBEQ	R3, R1, negPiPosInf

negPiPosInf:
	MOVD	$NegZero, R1
	MOVD	R1, ret+16(FP)
	RET

Note, that the comment is not correct with respect to the behavior, it actually returns -0 not Pi. In any case, Atan2(-Pi, +Inf) is not a special case, it just happens to be a test case for the actual special case Atan2(±y,+∞) = ±0

@golang golang locked and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@bmkessler @bcmills @gopherbot @mundaym