Skip to content

Conversation

sethaxen
Copy link
Member

@sethaxen sethaxen commented Jun 28, 2020

This PR fixes #215 by adding missing complex tests and where necessary generalizing rules and adding missing rules. New rules are

  • adjoint(::Number)
  • real(::Number)
  • imag(::Complex)
  • Complex(::Number)
  • Complex(::Number, ::Number)
  • hypot(::Complex)
  • hypot(::T, ::T) where T<:Union{Real,Complex}
  • exp(::Number)

It also simplifies the rules for abs, angle, and sign with shared utility functions.

I believe as of this PR, all scalar functions in Base for which we have complex rules should be tested on complex inputs.

@sethaxen
Copy link
Member Author

The one failure on Julia 1.4.2 is odd. muladd(x,y,z)'s rules are defined using @scalar_rule, which literally implements the primal for frule as muladd(x,y,z), yet somehow the equality check against muladd(x,y,z) fails for complex inputs (the values are still approximately equal). JuliaDiff/ChainRulesTestUtils.jl#46 relaxes the equality check, but that still doesn't explain why it fails here. I cannot reproduce this locally.

@willtebbutt
Copy link
Member

Hmm yeah, that is very strange... checking for approximate equality is fine though imho.

Comment on lines 2 to 13
@inline _realconjtimes(x, y) = real(conj(x) * y)
@inline _realconjtimes(x::Complex, y::Complex) = real(x) * real(y) + imag(x) * imag(y)
@inline _realconjtimes(x::Real, y::Complex) = x * real(y)
@inline _realconjtimes(x::Complex, y::Real) = real(x) * y
@inline _realconjtimes(x::Real, y::Real) = x * y

# imag(conj(x) * y) avoiding computing the real part if possible
@inline _imagconjtimes(x, y) = imag(conj(x) * y)
@inline _imagconjtimes(x::Complex, y::Complex) = -imag(x) * real(y) + real(x) * imag(y)
@inline _imagconjtimes(x::Real, y::Complex) = x * imag(y)
@inline _imagconjtimes(x::Complex, y::Real) = -imag(x) * y
@inline _imagconjtimes(x::Real, y::Real) = Zero()
Copy link
Member

Choose a reason for hiding this comment

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

should these all be AbstractComplex ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is an AbstractComplex in base.

@sethaxen
Copy link
Member Author

Okay this should be ready for review.

@MasonProtter I ended up simplifying the rules for abs, angle, and sign with shared utility functions. They pass the same test suite, and locally their runtime seem to be the same as the specialized versions you implemented, but it'd be nice if you could double check.

@MasonProtter
Copy link
Contributor

Looks good to me!

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Minor style thing. Otherwise LGTM

@sethaxen sethaxen merged commit 8134715 into JuliaDiff:master Jun 30, 2020
@sethaxen sethaxen deleted the complextests branch June 30, 2020 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing complex tests/rules for scalar functions

4 participants