Add LinearCombination transform #8

nicoleepp · 2021-02-02T00:08:13Z

Closes #5

Note that I couldn't get a linear combination using Statistics.mean to work. A lot of operations were returning NaNs. Also, that mean function expects a whole matrix of weights and not just the weights for each column. For these reasons, I opted not to define the CustomTransform instead of doing this explicitly.

From the issue description, I am unclear what the role of dims kwarg is or how it should work.

glennmoy · 2021-02-02T13:27:48Z

From the issue description, I am unclear what the role of dims kwarg is or how it should work.

I think dims would apply when you want the linear combination to only operate on a subset of columns.

e.g.

df = DataFrame(
    :time = Day(1):Day(1):Day(5),
    :a = [1, 2, 3, 4, 5],
    :b = [1, 1, 2, 3, 5],
)

lc = LinearCombination([1, 5])

Transforms.apply(df, lc; cols=[:a, :b])

similarly for dims on arrays.
Does this make sense?

bencottier · 2021-02-02T15:40:43Z

similarly for dims on arrays.

If it's selecting a subset of columns, then dims for arrays seems misleading - like it will apply the linear combination along a different axis. I think it should always be called cols for this usage.

bencottier

We should handle dims/cols as Glenn explained, but otherwise this looks good to me.

test/linear_combination.jl

nicoleepp · 2021-02-02T20:56:49Z

similarly for dims on arrays.

If it's selecting a subset of columns, then dims for arrays seems misleading - like it will apply the linear combination along a different axis. I think it should always be called cols for this usage.

I agree using dims would make me think it chooses only certain rows, not a subset of the array cols

codecov · 2021-02-02T23:11:02Z

Codecov Report

❗ No coverage uploaded for pull request base (main@e7ba638). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             main        #8   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?         5           
  Lines           ?        38           
  Branches        ?         0           
========================================
  Hits            ?        38           
  Misses          ?         0           
  Partials        ?         0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7ba638...1311db1. Read the comment docs.

nicoleepp · 2021-02-02T23:17:16Z

I have implemented cols as Ben suggested and left the dims tests commented out while we decide what dims should mean. I may just not understand what dims means for an Array but having both does seem confusing.

bencottier

Looks good to me. I don't think there is a use for dims as long as this transform is only applied on columns.

glennmoy · 2021-02-04T12:57:09Z

If it's selecting a subset of columns, then dims for arrays seems misleading - like it will apply the linear combination along a different axis. I think it should always be called cols for this usage.

Looks good to me. I don't think there is a use for dims as long as this transform is only applied on columns.

Sorry, that's dead right, in my first response I completely confused dims / cols.

But I think there is still a place for dims because we might not always take the linear combination column-wise.

e.g. consider just taking a sum over some matrix - this resembles a LinearCombination(ones(3)) in this case.

julia> A = [1 2 3; 4 5 6; 7 8 9];

julia> sum(A, dims=2)
3×1 Array{Int64,2}:
  6
 15
 24

julia> sum(A, dims=1)
1×3 Array{Int64,2}:
 12  15  18

I think all that needs to change is to change eachrow to eachslice(...; dims=dims)?

This means we would also have to change cols to inds/index/idx (or some better term).

src/linear_combination.jl

test/linear_combination.jl

src/linear_combination.jl

test/linear_combination.jl

src/linear_combination.jl

glennmoy · 2021-02-08T17:21:38Z

test/linear_combination.jl

+                d = Colon()
+                @test_throws ArgumentError Transforms.apply(A, lc; dims=d)
+            end
+
            @testset "dims = 1" begin


does this also work if passing in the axis key? e.g. foo, bar?

I checked it with Power on AxisKeys so it should work

https://github.com/invenia/Transforms.jl/blob/main/test/power.jl#L77

but I forgot to do the same with AxisArrays
https://github.com/invenia/Transforms.jl/blob/main/test/power.jl#L65

How do you do the same with AxisArrays though? If I use (Colon(), :foo, :bar) like for KeyedArray in test/power.jl I get

dims = foo: Error During Test at /Users/bencottier/JuliaEnvs/Transform.jl/test/power.jl:65 Got exception outside of a @test MethodError: no method matching length(::Symbol) Closest candidates are: length(!Matched::Base.Iterators.Flatten{Tuple{}}) at iterators.jl:1061 length(!Matched::Base.MethodList) at reflection.jl:872 length(!Matched::DataStructures.EnumerateAll) at /Users/bencottier/.julia/packages/DataStructures/ixwFs/src/multi_dict.jl:96 ... Stacktrace: [1] #eachslice#196 at ./abstractarraymath.jl:496 [inlined] [2] apply!(::AxisArray{Int64,2,Array{Int64,2},Tuple{Axis{:foo,Array{String,1}},Axis{:bar,Array{String,1}}}}, ::Power; dims::Symbol, kwargs::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /Users/bencottier/JuliaEnvs/Transform.jl/src/transformers.jl:57 [3] apply(::AxisArray{Int64,2,Array{Int64,2},Tuple{Axis{:foo,Array{String,1}},Axis{:bar,Array{String,1}}}}, ::Power; kwargs::Base.Iterators.Pairs{Symbol,Symbol,Tuple{Symbol},NamedTuple{(:dims,),Tuple{Symbol}}}) at /Users/bencottier/JuliaEnvs/Transform.jl/src/transformers.jl:64 [4] macro expansion at /Users/bencottier/JuliaEnvs/Transform.jl/test/power.jl:66 [inlined] [5] macro expansion at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1190 [inlined] [6] macro expansion at /Users/bencottier/JuliaEnvs/Transform.jl/test/power.jl:65 [inlined] [7] macro expansion at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115 [inlined] [8] macro expansion at /Users/bencottier/JuliaEnvs/Transform.jl/test/power.jl:62 [inlined] [9] macro expansion at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115 [inlined] [10] top-level scope at /Users/bencottier/JuliaEnvs/Transform.jl/test/power.jl:3 [11] include(::String) at ./client.jl:457 [12] top-level scope at /Users/bencottier/JuliaEnvs/Transform.jl/test/runtests.jl:12 [13] top-level scope at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115 [14] top-level scope at /Users/bencottier/JuliaEnvs/Transform.jl/test/runtests.jl:11 [15] include(::String) at ./client.jl:457 [16] top-level scope at none:6 [17] eval(::Module, ::Any) at ./boot.jl:331 [18] exec_options(::Base.JLOptions) at ./client.jl:272 [19] _start() at ./client.jl:506

it looks like eachslice doesn't work on AxisArrays maybe. Nevermind it was only a passing thought.

yeah it doesn't work without some extra voodoo

julia> eachslice(A, dims=axisdim(A, Axis{:foo})) Base.Generator{Base.OneTo{Int64},Base.var"#199#202"{AxisArray{Int64,2,Array{Int64,2},Tuple{Axis{:foo,Array{String,1}},Axis{:bar,Array{String,1}}}},Tuple{},Tuple{Colon}}}(Base.var"#199#202"{AxisArray{Int64,2,Array{Int64,2},Tuple{Axis{:foo,Array{String,1}},Axis{:bar,Array{String,1}}}},Tuple{},Tuple{Colon}}([1 2 3; 4 5 6], (), (Colon(),)), Base.OneTo(2))

And it's been on their todo list for 5 years 🙄
JuliaArrays/AxisArrays.jl#7

I can make this work if we want to, by checking if it's an AxisArray. I might prefer to do this in a follow up MR just to keep diffs smaller.

test/linear_combination.jl

Add LinearCombination transform

7947c66

nicoleepp requested a review from glennmoy February 2, 2021 00:08

Base automatically changed from gm/wip_interface to main February 2, 2021 11:45

bencottier reviewed Feb 2, 2021

View reviewed changes

test/linear_combination.jl Outdated Show resolved Hide resolved

Implement cols kwarg

5528c0b

bencottier approved these changes Feb 3, 2021

View reviewed changes

glennmoy reviewed Feb 4, 2021

View reviewed changes

src/linear_combination.jl Outdated Show resolved Hide resolved

src/linear_combination.jl Outdated Show resolved Hide resolved

src/linear_combination.jl Show resolved Hide resolved

glennmoy reviewed Feb 4, 2021

View reviewed changes

src/linear_combination.jl Outdated Show resolved Hide resolved

glennmoy mentioned this pull request Feb 4, 2021

Add Periodic transform #10

Merged

Add dims kwarg for matrices

2ca4296

nicoleepp commented Feb 5, 2021

View reviewed changes

src/linear_combination.jl Show resolved Hide resolved

src/linear_combination.jl Outdated Show resolved Hide resolved

test/linear_combination.jl Outdated Show resolved Hide resolved

morris25 reviewed Feb 5, 2021

View reviewed changes

src/linear_combination.jl Show resolved Hide resolved

src/linear_combination.jl Show resolved Hide resolved

glennmoy reviewed Feb 5, 2021

View reviewed changes

src/linear_combination.jl Outdated Show resolved Hide resolved

src/linear_combination.jl Show resolved Hide resolved

src/linear_combination.jl Outdated Show resolved Hide resolved

test/linear_combination.jl Outdated Show resolved Hide resolved

nicoleepp added 2 commits February 8, 2021 10:36

Use cols for tables, and restrict to Matrices

cc7a380

Update the LinearCombination docstring

c8d4735

glennmoy approved these changes Feb 8, 2021

View reviewed changes

Delete typo

1311db1

nicoleepp mentioned this pull request Feb 8, 2021

Make LinearCombination support specifying axis key as the dims? #13

Closed

nicoleepp merged commit 57cccbe into main Feb 8, 2021

nicoleepp deleted the ne/linear-combination branch February 8, 2021 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LinearCombination transform #8

Add LinearCombination transform #8

nicoleepp commented Feb 2, 2021

glennmoy commented Feb 2, 2021

bencottier commented Feb 2, 2021

bencottier left a comment

nicoleepp commented Feb 2, 2021 •

edited

Loading

codecov bot commented Feb 2, 2021 •

edited

Loading

nicoleepp commented Feb 2, 2021

bencottier left a comment

glennmoy commented Feb 4, 2021 •

edited

Loading

glennmoy Feb 8, 2021

glennmoy Feb 8, 2021 •

edited

Loading

nicoleepp Feb 8, 2021

bencottier Feb 8, 2021

glennmoy Feb 8, 2021

glennmoy Feb 8, 2021

nicoleepp Feb 8, 2021

nicoleepp Feb 8, 2021

Add LinearCombination transform #8

Add LinearCombination transform #8

Conversation

nicoleepp commented Feb 2, 2021

glennmoy commented Feb 2, 2021

bencottier commented Feb 2, 2021

bencottier left a comment

Choose a reason for hiding this comment

nicoleepp commented Feb 2, 2021 • edited Loading

codecov bot commented Feb 2, 2021 • edited Loading

Codecov Report

nicoleepp commented Feb 2, 2021

bencottier left a comment

Choose a reason for hiding this comment

glennmoy commented Feb 4, 2021 • edited Loading

glennmoy Feb 8, 2021

Choose a reason for hiding this comment

glennmoy Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

nicoleepp Feb 8, 2021

Choose a reason for hiding this comment

bencottier Feb 8, 2021

Choose a reason for hiding this comment

glennmoy Feb 8, 2021

Choose a reason for hiding this comment

glennmoy Feb 8, 2021

Choose a reason for hiding this comment

nicoleepp Feb 8, 2021

Choose a reason for hiding this comment

nicoleepp Feb 8, 2021

Choose a reason for hiding this comment

nicoleepp commented Feb 2, 2021 •

edited

Loading

codecov bot commented Feb 2, 2021 •

edited

Loading

glennmoy commented Feb 4, 2021 •

edited

Loading

glennmoy Feb 8, 2021 •

edited

Loading