Algorithms refactoring #18

lostella · 2019-02-22T09:51:31Z

This is a work in progress, but I thought it's better to open a CR already before we discuss about #17

Edit

Adding a bit of description. This PR is meant to:

remove any occurrence of Tuple{AbstractArray} in favor of just AbstractArray (with the idea of using ArrayPartition in place of Tuple, whenever this is needed)
restructure the iterables structure, splitting what is supposed to be immutable (that goes into the iterable) from what is mutable (that goes in the iteration state)
simplify the code of the algorithms, where possible (for example, in forward-backward-based iterations)
potentially update/unify the interface the different algorithms (in the sense of making homogeneous: similar structure of required vs optional arguments, similar requirements in terms of stepsizes vs Lipschitz constants, and so on)

Todo list:

Edit 2: this should fix #12, #16, and #19

nantonel · 2019-02-22T09:57:25Z

aren't these PR complementary somehow?

lostella · 2019-02-22T10:00:21Z

I think this clashes with #17 since it modifies the algorithms code in many places; on the other hand, this includes #17, since it also removes the dependency on tuples.

This said, it's perfectly fine for me to merge #17 (as soon as it works) and then I'll rebase this PR on top of that. I would just avoid doing unnecessary work, for example if making #17 work requires time and then this PR overwrites it almost completely.

nantonel · 2019-02-22T10:05:03Z

Well as you can see #17 does not have major changes. However perhaps it's safer to close #17 and directly remove BlockArrays in this PR.

nantonel · 2019-02-22T16:41:18Z

I see that you have the LBFGS operator here. Shouldn't we keep it in one place only? Personally I would prefer having it in AbstractOperators although I see that it belongs to this package as well.

lostella · 2019-03-02T23:08:23Z

@nantonel it's ok to duplicate code in this case in my opinion (since here it's not really user-facing) in favor of having more compact code: anyone wanting to dig into L-BFGS-based algorithms only needs to explore this package.

codecov-io · 2019-03-02T23:51:33Z

Codecov Report

Merging #18 into master will increase coverage by 1.52%.
The diff coverage is 93.91%.

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   92.41%   93.93%   +1.52%     
==========================================
  Files          12       12              
  Lines         593      445     -148     
==========================================
- Hits          548      418     -130     
+ Misses         45       27      -18

Impacted Files	Coverage Δ
src/ProximalAlgorithms.jl	`100% <ø> (+22.22%)`	⬆️
src/compat.jl	`0% <0%> (ø)`
src/utilities/fbetools.jl	`100% <100%> (ø)`
src/utilities/iterationtools.jl	`100% <100%> (ø)`
src/utilities/conjugate.jl	`100% <100%> (ø)`	⬆️
src/algorithms/davisyin.jl	`100% <100%> (ø)`
src/algorithms/douglasrachford.jl	`100% <100%> (ø)`
src/algorithms/primaldual.jl	`82.27% <82.27%> (ø)`
src/algorithms/zerofpr.jl	`94.59% <94.59%> (ø)`
src/algorithms/panoc.jl	`95.5% <95.5%> (ø)`
... and 12 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 b9a4085...1268a2d. Read the comment docs.

coveralls · 2019-03-22T22:12:44Z

Coverage decreased (-29.2%) to 60.599% when pulling f115ac0 on lostella:algorithms-refactoring into ad924db on kul-forbes:master.

coveralls · 2019-03-22T22:12:46Z

Coverage increased (+4.6%) to 93.98% when pulling 9267c9e on lostella:algorithms-refactoring into b9a4085 on kul-forbes:master.

lostella · 2019-04-14T13:24:15Z

I still need to finish up a bit with tests (it would be great to re-include tests using SeparableSum and ArrayPartition), but I'm fairly satisfied with the rest of the refactoring.

I would kindly ask reviews from @nantonel (in particular: changes in the algorithms APIs and their implications on higher level packages, i.e. StructuredOptimization.jl) and @pylat (in particular: primal-dual algorithms and tests).

pylat

Looks quite nice and clean. I must confess, I didn't check thoroughly. Everything that I checked seems to be in order. I am annoyed by this ugly stepsize selection but I remember that it was inevitable.

lostella · 2019-04-17T08:11:27Z

Thanks, Puya. I think the default selection for gamma1 and gamma2 is fine, however complicated, as long as it's well separated from the rest of the code (interface, initialization, iteration). It's also much better than having people have to manually select gamma1 and gamma2, of course :-)

nantonel

Thanks for this hard work! I've made some minor comments: I suppose a couple of allocations could be reduced and some type instability fixed (especially in DRS family) but perhaps we can leave this to future PRs. I suppose we could enable verbose in some tests, should slightly improve coverage? Before tagging a new version, let's wait and fix StructOpt first.

REQUIRE

src/utilities/arraypartition.jl

nantonel · 2019-05-10T06:34:49Z

src/algorithms/douglasrachford.jl

+
+struct DRS_iterable{R <: Real, C <: Union{R, Complex{R}}, T <: AbstractArray{C}}
+    f
+    g


f and g shouldn't be typed? of course this can be fixed on a later PR.

Of course they should. And I should be less lazy :-p

I'm adding all these to the TODO list on top, and will add the changes before merging

nantonel · 2019-05-10T06:40:05Z

src/algorithms/forwardbackward.jl

+end
+
+function Base.iterate(iter::FBS_iterable{R}, state::FBS_state{R, Tx, TAx}) where {R, Tx, TAx}
+    Az, f_Az, grad_f_Az, At_grad_f_Az = nothing, nothing, nothing, nothing


but this could be initialised already when constructing state in Base.iterate(iter::FBS_iterable{R})?
Or am I wrong?

You mean having additional fields for these in the state? Yes, that could be, that could save some allocations I guess?

I think I've addressed everything except for this: I would open a separate issue and leave it to a separate PR, from what I've seen algorithms perform already very good like this

nantonel · 2019-05-10T06:46:34Z

src/algorithms/panoc.jl

+end
+
+function Base.iterate(iter::PANOC_iterable{R}, state::PANOC_state{R, Tx, TAx}) where {R, Tx, TAx}
+    Az, f_Az, grad_f_Az, At_grad_f_Az = nothing, nothing, nothing, nothing


same comment as above

nantonel · 2019-05-10T06:46:58Z

src/algorithms/primaldual.jl

+    g
+    h
+    l
+    L


same as DRS

lostella · 2019-05-11T07:41:00Z

@nantonel I think coverage is OK now: one thing to do is removing Coveralls also here, since it is more confusing than useful.

Of course we’ll wait before tagging: we first need to understand how package registration now works :D

…t.toml

lostella mentioned this pull request Feb 22, 2019

Removing BlockArrays #17

Merged

lostella force-pushed the algorithms-refactoring branch from e1dc19f to eb85b0a Compare March 2, 2019 22:56

nantonel mentioned this pull request Mar 7, 2019

Remove some dependencies #19

Closed

3 tasks

lostella force-pushed the algorithms-refactoring branch from 6581d1b to f115ac0 Compare March 22, 2019 21:24

lostella mentioned this pull request Apr 4, 2019

strange bug kul-optec/PANOC.jl#1

Closed

lostella requested review from nantonel and pylat April 14, 2019 13:24

pylat approved these changes Apr 16, 2019

View reviewed changes

nantonel approved these changes May 10, 2019

View reviewed changes

lostella added 8 commits May 11, 2019 13:04

first batch of changes

561bde3

fixing a test

2655248

fixed filenames case (thank you, macos)

0334128

added compatibility with julia-1.0

7f7c2fb

added DYS, fixed typing issues, reorganized tests

d745ddc

fixed typing errors

c6c74ba

refactored zerofpr

a3f08fc

updated README, removed dependency, removed REQUIREs and added Projec…

8e02008

…t.toml

lostella force-pushed the algorithms-refactoring branch from 94a55c2 to 8e02008 Compare May 11, 2019 12:19

disabling coveralls

9267c9e

lostella added 6 commits May 11, 2019 15:37

added types to problem terms

8e6e444

fixed bugs, added tests using ArrayPartition and AbstractOperators

03ef797

reorganized imports in tests

f390499

removed incompatibility with julia 1.0

cccd183

conforming function names to the coding style guide

2acf59d

updated docstrings and references

1268a2d

lostella merged commit 09ccc63 into JuliaFirstOrder:master May 12, 2019

lostella mentioned this pull request May 12, 2019

fix lower bound to Lipschitz constant #16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Algorithms refactoring #18

Algorithms refactoring #18

lostella commented Feb 22, 2019 •

edited

nantonel commented Feb 22, 2019

lostella commented Feb 22, 2019

nantonel commented Feb 22, 2019

nantonel commented Feb 22, 2019

lostella commented Mar 2, 2019

codecov-io commented Mar 2, 2019 •

edited

coveralls commented Mar 22, 2019

coveralls commented Mar 22, 2019 •

edited

lostella commented Apr 14, 2019

pylat left a comment

lostella commented Apr 17, 2019

nantonel left a comment

nantonel May 10, 2019

lostella May 10, 2019 •

edited

nantonel May 10, 2019

lostella May 10, 2019

lostella May 11, 2019

nantonel May 10, 2019

nantonel May 10, 2019

lostella commented May 11, 2019

Algorithms refactoring #18

Algorithms refactoring #18

Conversation

lostella commented Feb 22, 2019 • edited

nantonel commented Feb 22, 2019

lostella commented Feb 22, 2019

nantonel commented Feb 22, 2019

nantonel commented Feb 22, 2019

lostella commented Mar 2, 2019

codecov-io commented Mar 2, 2019 • edited

Codecov Report

coveralls commented Mar 22, 2019

coveralls commented Mar 22, 2019 • edited

lostella commented Apr 14, 2019

pylat left a comment

Choose a reason for hiding this comment

lostella commented Apr 17, 2019

nantonel left a comment

Choose a reason for hiding this comment

nantonel May 10, 2019

Choose a reason for hiding this comment

lostella May 10, 2019 • edited

Choose a reason for hiding this comment

nantonel May 10, 2019

Choose a reason for hiding this comment

lostella May 10, 2019

Choose a reason for hiding this comment

lostella May 11, 2019

Choose a reason for hiding this comment

nantonel May 10, 2019

Choose a reason for hiding this comment

nantonel May 10, 2019

Choose a reason for hiding this comment

lostella commented May 11, 2019

lostella commented Feb 22, 2019 •

edited

codecov-io commented Mar 2, 2019 •

edited

coveralls commented Mar 22, 2019 •

edited

lostella May 10, 2019 •

edited