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

Add missing complex tests and rules #216

Merged
merged 31 commits into from
Jun 30, 2020
Merged

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

src/rulesets/Base/fastmath_able.jl Outdated Show resolved Hide resolved
Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>
@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