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

Change SVD to use Composite #157

Merged
merged 4 commits into from
Jan 22, 2020
Merged

Change SVD to use Composite #157

merged 4 commits into from
Jan 22, 2020

Conversation

nickrobinson251
Copy link
Contributor

@nickrobinson251 nickrobinson251 commented Jan 20, 2020

@willtebbutt
Copy link
Member

Very excited for replacing zero(large_array) with Zero().

src/ChainRules.jl Outdated Show resolved Hide resolved
src/rulesets/LinearAlgebra/factorization.jl Outdated Show resolved Hide resolved
src/rulesets/LinearAlgebra/factorization.jl Outdated Show resolved Hide resolved
src/helper_functions.jl Outdated Show resolved Hide resolved
@nickrobinson251
Copy link
Contributor Author

when returning Zero test fails with comparing to finite differences (which gives an array of approx zeros)

julia> extern(Zero())  zeros(2, 2)
ERROR: MethodError: no method matching isapprox(::Bool, ::Array{Float64,2})
Closest candidates are:
  isapprox(::Missing, ::Any; kwargs...) at missing.jl:90
  isapprox(::Any, ::Missing; kwargs...) at missing.jl:91
  isapprox(::Number, ::Number; atol, rtol, nans) at floatfuncs.jl:274
  ...
Stacktrace:
 [1] top-level scope at REPL[15]:1

julia> extern(Zero()) == zeros(2, 2)
false

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Jan 21, 2020

i guess i can just write the tests to account for this, i.e. compare element-wise, but the output on failure is less good:

# now; where in practice we have `Zero` instead of `zeros(2, 2)`
julia> @test all(isapprox.(zeros(2, 2), ones(2, 2)))
Test Failed at REPL[34]:1
  Expression: all(isapprox.(zeros(2, 2), ones(2, 2)))
ERROR: There was an error during testing

# before; better message but didn't work with `Zero` instead of `zeros(2, 2)`
julia> @test isapprox(zeros(2, 2), ones(2, 2))
Test Failed at REPL[35]:1
  Expression: isapprox(zeros(2, 2), ones(2, 2))
   Evaluated: isapprox([0.0 0.0; 0.0 0.0], [1.0 1.0; 1.0 1.0])
ERROR: There was an error during testing

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Jan 21, 2020

The current version of this PR is what it would look like if we extended various linear algebra function to handle Zero

(We do already extend conj)

This maybe leaves some optimizations still up for grabs, but it's a lot cleaner than naively checking A isa Zero many places.

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Jan 21, 2020

What's up with Compostie and accumulate? What's up with accumulate at all? (xref JuliaDiff/ChainRulesCore.jl#62)

is accumulate[!](x::C, y::C) where c<:Composite = C(; elementwise_add(backing(x), backing(y))...) or ??

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.

I can't see anything to disagree with here.

src/rulesets/LinearAlgebra/factorization.jl Outdated Show resolved Hide resolved
@nickrobinson251 nickrobinson251 changed the title WIP Change SVD NamedTuple to Composite Change SVD to use Composite Jan 21, 2020
@oxinabox oxinabox reopened this Jan 22, 2020
test/runtests.jl Outdated Show resolved Hide resolved
Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Other than the stuff commented this LGTM

# Deal with name clashes, by defining in this module which one we mean.
const accumulate = ChainRulesCore.accumulate
const accumulate! = ChainRulesCore.accumulate!
using ChainRulesCore: AbstractZero
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nickrobinson251 and others added 3 commits January 22, 2020 18:21
Remove mis-use of InplaceableThunk

Move test utlitity to test_util file

Use unthunk instead of extern

Remove unnecessary thunking

Extend LinearAlgebra to handle Zero

Change NamedTuple to Composite in accumulate! test

Use accumulate from ChainRulesCore #110

Clean-up TODOs

Replace `accumulate` with `+`

Update to use `AbstractZero`

Co-Authored-By: Lyndon White <oxinabox@ucc.asn.au>

Add note about MulAddMacro

Co-Authored-By: Lyndon White <oxinabox@ucc.asn.au>

Apply PR review suggestions
* Remove `store!`, `accumulate`, `accumulate!`

* Remove `test_accumulate` functions
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.

None yet

3 participants