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

Introduce Composite for structured deriviatives #59

Merged
merged 4 commits into from
Nov 28, 2019
Merged

Introduce Composite for structured deriviatives #59

merged 4 commits into from
Nov 28, 2019

Conversation

oxinabox
Copy link
Member

This will finally finish resolving #8

I think before it is done it needs unthunk to be used instead of extern #56

Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

I am enjoying following along :)

src/composite_backing.jl Outdated Show resolved Hide resolved
src/composite_backing.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Member Author

oxinabox commented Nov 11, 2019

Spoke to @MikeInnes and co. in the ML call last week.
If an inner constructor is not provided,
we should not bypass the lack,
because it may encode an invarient.

So we ask the user what to do, they can either provide a default contructor,
overload ChainRulesCore.construct(::Type{Primal}, ::NamedTuple)
or overload +(::Primal, ::Composite{Primal})

Except for closures.
Closures have no constructors, for performance reasons.
Jeff says it is fine to construct them via the same way Serialization does.
but we can add them in a follow up PR.
The Closure case is not hugely interesting for us, since it is pretty weird to have a handle on one to be able to define its deriviative

@oxinabox oxinabox changed the title WIP: Introduce Composite for structured deriviatives RFC: Introduce Composite for structured deriviatives Nov 11, 2019
Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

took a quick look and seems grand. excited to see this happening. thanks a lot and sorry i don't have more time to review

src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/differentials.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Member Author

sorry i don't have more time to review

You reviewed this at 8pm.

src/differentials.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
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.

Broadly very nice. It would be nice to see some unit tests for the various bits and bobs in src/composite_core.jl in a corresponding file in test, as opposed to just integration tests via test/differentials.jl. Also, some tests with nested composites probably wouldn't be a bad idea.

src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Member Author

oxinabox commented Nov 21, 2019

I put some effort to use colors to make it a good log message when you do addition and it can't make a primal.

In this example StructWithInvarient is the type of value, so the user should know what that is

screenshot of colorred log messag

rather than that all being in red

src/ChainRulesCore.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/composite_core.jl Outdated Show resolved Hide resolved
src/differentials.jl Outdated Show resolved Hide resolved
src/differentials.jl Show resolved Hide resolved
@mattBrzezinski
Copy link
Member

Looks good to me :)

@willtebbutt
Copy link
Member

Trying to use Composite to work with NamedTuples:

A = (a=rand(5), b=rand(5, 5))
dA = Composite{typeof(A)}(;A...)
2dA # this works :)
A + dA # this errors :(

the last line yields

julia> A + dA
ERROR: Could not construct NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}} after addition.
This probably means no default constructor is defined.
Either define a default constructor
NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}}(1, 2)
or overload
ChainRulesCore.construct(::Type{NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}}}, ::Composite{NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}},Tuple{Array{Float64,1},Array{Float64,2}}})
or overload
Base.:+(::NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}}, ::Composite{NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}},Tuple{Array{Float64,1},Array{Float64,2}}})
Original Exception:
MethodError(ChainRulesCore.elementwise_add, ((a = [0.6656951461385991, 0.4645935051577579], b = [0.13576750972690776 0.19906834130647622; 0.22171335040027196 0.6481124619162637]), ([0.6656951461385991, 0.4645935051577579], [0.13576750972690776 0.19906834130647622; 0.22171335040027196 0.6481124619162637])), 0x0000000000006886)

Stacktrace:
 [1] +(::NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}}, ::Composite{NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}},Tuple{Array{Float64,1},Array{Float64,2}}}) at /scratch/will/ml/IWVIHilbertGP/dev/VIEstimators/dev/ChainRulesCore/src/differential_arithmetic.jl:109
 [2] top-level scope at REPL[31]:1
caused by [exception 1]
MethodError: no method matching elementwise_add(::NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}}, ::Tuple{Array{Float64,1},Array{Float64,2}})
Closest candidates are:
  elementwise_add(::Tuple, ::Tuple) at /scratch/will/ml/IWVIHilbertGP/dev/VIEstimators/dev/ChainRulesCore/src/composite_core.jl:86
  elementwise_add(::NamedTuple{an,T} where T<:Tuple, ::NamedTuple{bn,T} where T<:Tuple) where {an, bn} at /scratch/will/ml/IWVIHilbertGP/dev/VIEstimators/dev/ChainRulesCore/src/composite_core.jl:93
Stacktrace:
 [1] +(::NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}}, ::Composite{NamedTuple{(:a, :b),Tuple{Array{Float64,1},Array{Float64,2}}},Tuple{Array{Float64,1},Array{Float64,2}}}) at /scratch/will/ml/IWVIHilbertGP/dev/VIEstimators/dev/ChainRulesCore/src/differential_arithmetic.jl:107
 [2] top-level scope at REPL[31]:1

Am I using the interface incorrectly, or is this a bug? I can't see any NamedTuple tests, so perhaps it's a bug?

@oxinabox
Copy link
Member Author

Yeah, I never tested it with NamedTuple as primal so could well be a bug.
Will add tests then investigate

@oxinabox oxinabox changed the title RFC: Introduce Composite for structured deriviatives Introduce Composite for structured deriviatives Nov 28, 2019
more composite stuff

Work on constructing

wip break things up

make elementwise add

Fixup tests mostly

fix comment

how about we don't just stackoverflow when we do getproperty

add good tests

add tests (pun intended)

more tests

fix scaling tests

add error

Update src/composite_core.jl

Co-Authored-By: Nick Robinson <npr251@gmail.com>

Update src/differentials.jl

Co-Authored-By: Nick Robinson <npr251@gmail.com>

Update src/differential_arithmetic.jl

Co-Authored-By: Mathieu Besançon <mathieu.besancon@gmail.com>

Update src/differential_arithmetic.jl

resolve comments from PR

add tests on indexing, iterating, and properties

fix eltype

test extern

check on allocations and backing behavour

test about internals

delete ambi tests because we intentionally have some now

use P rather than Primal for typevar

Update src/composite_core.jl

sort importand and exports

remove extra blank lines

move comment

Fix name tuple construction
@oxinabox oxinabox merged commit 6184811 into master Nov 28, 2019
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.

6 participants