Using RecursiveArrayTools #8

nantonel · 2019-01-29T17:15:48Z

Addressing #5 and #7 and #9

I'm convinced that removing completely operations with Tuples is the way to go, like these ones:

A*(x,y) -> A*ArrayParition(x,y)
mul!(y::Tuple, A::AbstractOperators, b::Tuple) -> mul!(y::ArrayParition, A::AbstractOperators, b::ArrayParition)
etc..

The reason is that the notation compatibility between Tuples and ArrayPartitions isn't the best.
ArrayPartition employs a different redefinition of getindex: if typeof(x) <: ArrayParition, x[1] will not give you the first array of the collection but the first element of the first array. So to avoid confusion in the code I would avoid working with Tuples of Arrays at all.

That being said, I'm quite in favor of using ArrayParitions: although this will mean breaking changes in StruturedOptimization, it will possibly lead to many simplifications since ArrayPartion <: AbstractArray. We can already see some in the LBFGS (where dots can now be employed) and in Sum.

TODO: if we decide to completely remove multiplication with Tuples (which I think is the way to go), update docs accordingly

lostella · 2019-02-12T20:22:06Z

I would take the occasion to remove Julia 0.7 from Travis, and putting 1.1 instead

lostella

Looks like there are more occurrences of Tuple{AbstractArray} around to get rid of.

But in general adopting ArrayPartition seems to lead to much simpler code in many parts already!

lostella · 2019-02-12T20:26:58Z

src/AbstractOperators.jl

       NonLinearOperator,
       AbstractOperator
+# from other packages
+export ArrayPartition, mul!


Why these exports?

they are not necessary, but quite practical when you load the package to test stuff.

I would remove exporting ArrayPartition for sure, I think the package should export what it defines. Maybe mul! could still be exported

lostella · 2019-02-12T20:33:07Z

src/calculus/HCAT.jl

+            N, # number of AbstractOperator 
+            L <: NTuple{N,AbstractOperator},
+            P <: NTuple{N,Union{Int,Tuple}},
+            C <: Union{NTuple{M,AbstractArray}, AbstractArray},


I think this one could be simplified? In the idea of moving to ArrayPartition to represent product spaces, here the arrival space can just be an AbstractArray, since ArrayPartition <: AbstractArray. Or am I missing something?

yes I guess it can be simplified, was just lazy! 😄

lostella · 2019-02-12T20:34:32Z

src/calculus/HCAT.jl

+            C <: Union{NTuple{M,AbstractArray}, AbstractArray},
+           } <: AbstractOperator
 	A::L     # tuple of AbstractOperators
 	idxs::P  # indices 


Also, I don’t remember what the advantage of storing these indices is, but I guess that storing the exact Tuple of operators to be horizontally concatenated could potentially simplify the code?

The indices are due to eventual rearrangement of block of variables (I guess we should call them ArrayPartitions now). I suppose stuff can be simplified there as well, but it's out of the scope of this PR.

lostella · 2019-02-12T20:37:35Z

src/linearoperators/LBFGS.jl

+	s_M = [0 .*similar(x) for i = 1:M]
+	y_M = [0 .*similar(x) for i = 1:M]
+	s = 0 .*similar(x)
+	y = 0 .*similar(x)


You can just use zero(x) in all these here

zero(x::AbstractArray) is no longer in use since Julia 1.0

It’s there, and I think it was introduced in 0.7: https://docs.julialang.org/en/v1/base/numbers/#Base.zero

ah right! in the past it was zeros now it's zero, that's why I got confused!

nantonel · 2019-02-13T08:02:09Z

I would take the occasion to remove Julia 0.7 from Travis, and putting 1.1 instead

I did that in the REQUIRE, don't know why Travis still checked 0.7! I've put 1.0 should I put 1.1? I never remember this stuff!

lostella · 2019-02-13T08:04:54Z

Just remove 0.7 from .travis.yml, and add 1.1

nantonel · 2019-02-13T08:10:31Z

Ah right! It's not in the REQUIRE! Anyway, before merging this PR we should fix the other packages as well.

codecov-io · 2019-02-13T15:15:11Z

Codecov Report

Merging #8 into master will decrease coverage by 2.83%.
The diff coverage is 83.41%.

@@            Coverage Diff            @@
##           master      #8      +/-   ##
=========================================
- Coverage   87.04%   84.2%   -2.84%     
=========================================
  Files          45      45              
  Lines        1497    1475      -22     
=========================================
- Hits         1303    1242      -61     
- Misses        194     233      +39

Impacted Files	Coverage Δ
src/AbstractOperators.jl	`100% <ø> (ø)`	⬆️
src/calculus/Sum.jl	`89.13% <100%> (-1.78%)`	⬇️
src/calculus/Reshape.jl	`96.15% <100%> (+4.48%)`	⬆️
src/properties.jl	`88.88% <100%> (+1.08%)`	⬆️
src/calculus/Hadamard.jl	`93.1% <100%> (-0.23%)`	⬇️
src/calculus/Jacobian.jl	`75% <100%> (-8.34%)`	⬇️
src/syntax.jl	`91.48% <100%> (+0.58%)`	⬆️
src/calculus/NonLinearCompose.jl	`100% <100%> (+2.7%)`	⬆️
src/calculus/VCAT.jl	`56.19% <42%> (-0.45%)`	⬇️
src/calculus/DCAT.jl	`89.87% <71.42%> (ø)`	⬆️
... and 6 more

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 087f8cc...c110542. Read the comment docs.

codecov-io · 2019-02-13T15:15:11Z

Codecov Report

Merging #8 into master will decrease coverage by 2.76%.
The diff coverage is 83.41%.

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
- Coverage   87.04%   84.27%   -2.77%     
==========================================
  Files          45       45              
  Lines        1497     1475      -22     
==========================================
- Hits         1303     1243      -60     
- Misses        194      232      +38

Impacted Files	Coverage Δ
src/AbstractOperators.jl	`100% <ø> (ø)`	⬆️
src/calculus/Sum.jl	`89.13% <100%> (-1.78%)`	⬇️
src/calculus/Reshape.jl	`96.15% <100%> (+4.48%)`	⬆️
src/properties.jl	`88.88% <100%> (+1.08%)`	⬆️
src/calculus/Hadamard.jl	`93.1% <100%> (-0.23%)`	⬇️
src/calculus/Jacobian.jl	`75% <100%> (-8.34%)`	⬇️
src/syntax.jl	`91.48% <100%> (+0.58%)`	⬆️
src/calculus/NonLinearCompose.jl	`100% <100%> (+2.7%)`	⬆️
src/calculus/VCAT.jl	`56.19% <42%> (-0.45%)`	⬇️
src/calculus/DCAT.jl	`89.87% <71.42%> (ø)`	⬆️
... and 6 more

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 087f8cc...a1b9736. Read the comment docs.

.travis.yml

appveyor.yml

src/AbstractOperators.jl

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

test/utils.jl

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

lostella

Good to go!

nantonel added 2 commits January 23, 2019 15:45

using ArrayPartitions

98f7114

using ArrayPartition - fixed linear calc

3de5a1a

nantonel requested a review from lostella January 29, 2019 17:16

nantonel added 2 commits February 12, 2019 17:45

fixed nonlinear stuff as well

e3112dd

drop julia 0.7

8ab94e6

lostella reviewed Feb 12, 2019

View reviewed changes

corrections/simplifications

c110542

lostella reviewed Feb 18, 2019

View reviewed changes

.travis.yml Show resolved Hide resolved

appveyor.yml Show resolved Hide resolved

src/AbstractOperators.jl Outdated Show resolved Hide resolved

src/AbstractOperators.jl Show resolved Hide resolved

lostella and others added 3 commits February 19, 2019 08:41

Update .travis.yml

965a246

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

Update appveyor.yml

300c74f

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

Update src/AbstractOperators.jl

5a2c477

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

This was referenced Feb 22, 2019

Removing BlockArrays JuliaFirstOrder/ProximalAlgorithms.jl#17

Merged

Removing BlockArrays JuliaFirstOrder/StructuredOptimization.jl#17

Merged

lostella reviewed Feb 26, 2019

View reviewed changes

test/utils.jl Outdated Show resolved Hide resolved

test/utils.jl Outdated Show resolved Hide resolved

test/utils.jl Outdated Show resolved Hide resolved

test/utils.jl Outdated Show resolved Hide resolved

test/utils.jl Outdated Show resolved Hide resolved

lostella and others added 5 commits February 27, 2019 09:58

Update test/utils.jl

fe457a8

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

Update test/utils.jl

a270dd4

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

Update test/utils.jl

9121ec8

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

Update test/utils.jl

3be373e

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

Update test/utils.jl

4d669b1

Co-Authored-By: nantonel <nantonel@esat.kuleuven.be>

lostella approved these changes Feb 27, 2019

View reviewed changes

nantonel added 2 commits March 6, 2019 16:53

fixing docs

db1e8d2

whops! testing stuff in travis

a1b9736

nantonel merged commit 7cf2a82 into kul-optec:master Mar 6, 2019

Using RecursiveArrayTools #8

Using RecursiveArrayTools #8

Uh oh!

Conversation

nantonel commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lostella commented Feb 12, 2019

Uh oh!

lostella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nantonel commented Feb 13, 2019

Uh oh!

lostella commented Feb 13, 2019

Uh oh!

nantonel commented Feb 13, 2019

Uh oh!

codecov-io commented Feb 13, 2019

Codecov Report

Uh oh!

codecov-io commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lostella left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nantonel commented Jan 29, 2019 •

edited

Loading

codecov-io commented Feb 13, 2019 •

edited

Loading