Exponential Cone #29

tjdiamandis · 2020-12-23T20:06:53Z

Projection onto exponential cone and its derivative.

The function _in_exp_cone(v::AbstractVector{T}; dual=false) may be necessary and incorporated into the distance function. However, in the projection I allow some tolerance for the points close to the boundary.

update fork

fixed dependency conflicts; added NonlinearSolve, JuMP, SCS Merge branch 'master' of github.com:tjdiamandis/MathOptSetDistances.jl into master

codecov · 2020-12-23T20:11:36Z

Codecov Report

Merging #29 (edad4d4) into master (3af4edb) will increase coverage by 1.10%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   88.16%   89.26%   +1.10%     
==========================================
  Files           5        6       +1     
  Lines         355      438      +83     
==========================================
+ Hits          313      391      +78     
- Misses         42       47       +5

Impacted Files	Coverage Δ
src/MathOptSetDistances.jl	`100.00% <ø> (ø)`
src/distance_sets.jl	`72.25% <50.00%> (-0.59%)`	⬇️
src/utils.jl	`94.11% <94.11%> (ø)`
src/projections.jl	`98.78% <96.29%> (-1.22%)`	⬇️
src/chainrules.jl	`100.00% <0.00%> (ø)`

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 3af4edb...edad4d4. Read the comment docs.

src/projections.jl

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

Project.toml

src/projections.jl

matbesancon · 2020-12-23T21:13:17Z

A major drawback is the addition of a dependency here on NonlinearSolve. Ideally this package will be used by others requiring set distances and projections so we should avoid bringing in a lot of direct and transitive dependencies)

src/projections.jl

joaquimg

Great job!

joaquimg · 2020-12-24T15:42:25Z

src/projections.jl

+function projection_on_set(::DefaultDistance, v::AbstractVector{T}, s::MOI.ExponentialCone) where {T}
+    _check_dimension(v, s)
+
+    if distance_to_set(DefaultDistance(), v, MOI.ExponentialCone()) < 1e-8


I think distances should use projections more than the other way around.

What's the advantage of using projections for distances?

If we fall into case 4 for the exponential cone projection, i.e., (x,y,z) not in K, -K*, and (x > 0 or y > 0), then computing the projection is going to take significantly longer than computing the distance. A few microseconds vs 10s of nanoseconds.

What's the advantage of using projections for distances?

distance can be seen as a composition of the projection with a point-to-point distance. What we need here to be precise is the set membership for the exp cone and its polar. I advised using distance for this and checking distance approx 0.

@joaquimg do you think we should use another membership function here? The motivation for re-using distance was avoiding duplication

This might be the first version of having two distances.
Because a very common distance is the euclidean.
But euclidean requires the projection first.
Our current distance is some epigraphical distance which is good for boolean checking if the point is the the cone.
I don't we should remove the current distance, I would just rename them and have both.
One could be used inside the the other, but I would use euclidean by default. Actually, whenever I possible I think we should use euclidean to have is as uniform as possible.

@tjdiamandis to simplify your work for now, you can just implement the quick check _in_exponential(v) that you had before.
Sorry for making you backtrack on this. I'll refactor it with the additional distance in another PR

No worries! Sounds good. Will do today.

We can just open an issue and let it be like this for now.

see here: #32

joaquimg · 2020-12-24T15:51:26Z

Project.toml

@@ -23,7 +23,9 @@ julia = "1"

 [extras]
 ChainRulesTestUtils = "cdddcdb0-9152-4a09-a978-84456f9df70a"
+JuMP = "4076af6c-e467-56ae-b986-b466b2749572"
+SCS = "c946c3f1-0d1f-5ce8-9dea-7daa1f7e2d13"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"


Dont we need Random here?

I saw that Random was already in the package dependencies, but I don't think it's used outside of tests. I can move it to extras, unless there's something I'm missing.

I'll do that in a following PR, we can leave it in the deps for now

src/projections.jl

matbesancon · 2020-12-25T20:41:04Z

src/utils.jl

@@ -0,0 +1,37 @@
+
+function _bisection(f, left, right; max_iters=10000, tol=1e-10)


The code in this function will yield many iterations or not converge because of strict inequality check, one should use:
a ≈ b atol=tol instead of equality checks for floating-point numbers

Originally designed to hit floating point limit (since intervals are relatively small), but I agree that this level of precision probably yields too many iterations. Will change to abs(f(mid)) < tol for early stop (similar to lines 32-33 for max_iter stop check).

Originally designed to hit floating point limit

yes but here you ask for strict equality right?

Yes, so the algorithm stops when the boundary is indistinguishable from the midpoint to floating point precision. E.g., 1 == 1 + eps()/2 evaluates to true.

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

test/projection_gradients.jl

matbesancon · 2020-12-25T21:27:56Z

test/projections.jl

+        @variable(model, z[1:3])
+        @variable(model, t)
+        @objective(model, Min, t)
+        @constraint(model, sum((x-z).^2) <= t)


Random note:
interesting, I didn't think this way of writing the squared norm would be faster than dot:

julia> @btime @constraint($model, sum(($x-$z).^2) <= $t) 4.522 μs (112 allocations: 9.39 KiB) julia> @btime @constraint($model, ($x-$z) ⋅ ($x-$z) <= $t) 6.419 μs (168 allocations: 13.28 KiB)

Oh interesting. Admittedly, I didn't think to check the latter.

src/projections.jl

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

tjdiamandis · 2020-12-25T22:12:22Z

Sorry for the type return errors. Thanks so much for the thorough review! I understand the general design principle now.

matbesancon · 2020-12-25T22:16:19Z

no problem, thanks a lot for the work. This is also me failing to catch everything at once, hence the back and forth which can feel tedious.

For diagm, the issue is that Julia 1.0 did not have the default diagm(v) = diagm(0 => v) yet, with 0 the convention to indicate the main diagonal of the matrix

matbesancon · 2020-12-25T22:25:12Z

Tests look green, we'll let the PR there for a day if other folks want to take a look or add something, but looks good on my side!

src/utils.jl

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

Merge branch 'master' of github.com:tjdiamandis/MathOptSetDistances.jl into master

matbesancon · 2020-12-27T11:10:07Z

Thanks again for the PR!

tjdiamandis added 11 commits December 23, 2020 10:16

Merge pull request #1 from matbesancon/master

2fb3701

update fork

add NonlinearSolve dep

906e784

add NonlinearSolve

45bac58

add Random, JuMP, SCS for tests

f21cded

add exp cone projection

052dbe1

add exp cone proj tests

ba899b8

merge changes

4bf77c4

fixed dependency conflicts; added NonlinearSolve, JuMP, SCS Merge branch 'master' of github.com:tjdiamandis/MathOptSetDistances.jl into master

comment out error message

3ce2c5e

derivatives for exp cone

7ead9ad

tests for Dproj_Kexp

48d3fcf

add check for bisection interval

52fd09f

matbesancon reviewed Dec 23, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

matbesancon reviewed Dec 23, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

matbesancon reviewed Dec 23, 2020

View reviewed changes

src/projections.jl Show resolved Hide resolved

tjdiamandis and others added 2 commits December 23, 2020 15:29

remove link

1988359

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

update to use _check_dimension

cc830fc

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

matbesancon reviewed Dec 23, 2020

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

tjdiamandis added 2 commits December 23, 2020 15:36

move JuMP, SCS to test only

2070ab1

check dim with _check_dimension

b1e922e

matbesancon reviewed Dec 23, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

fix loading of SCS and JuMP test dep

773bf8c

matbesancon reviewed Dec 23, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

tjdiamandis marked this pull request as draft December 23, 2020 21:26

odow reviewed Dec 24, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

tjdiamandis added 3 commits December 23, 2020 23:28

remove NonlinearSolve dep

82fc6f9

use distance_to_set to check membership

b1fa666

shortcircuit cases for exp cone

4e12347

joaquimg reviewed Dec 24, 2020

View reviewed changes

matbesancon reviewed Dec 25, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

matbesancon reviewed Dec 25, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

matbesancon reviewed Dec 25, 2020

View reviewed changes

tjdiamandis and others added 4 commits December 25, 2020 15:44

Update src/distance_sets.jl

3912710

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

Update src/distance_sets.jl

276ca15

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

Update src/projections.jl - code review

7f37235

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

Update src/projections.jl - code review

c97645a

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

matbesancon mentioned this pull request Dec 25, 2020

Distances and membership for exponential cone #32

Open

update bisection rootfinding

25d8518

matbesancon reviewed Dec 25, 2020

View reviewed changes

test/projection_gradients.jl Outdated Show resolved Hide resolved

remove print on error

1b24cf6

matbesancon reviewed Dec 25, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

matbesancon reviewed Dec 25, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

matbesancon reviewed Dec 25, 2020

View reviewed changes

src/projections.jl Outdated Show resolved Hide resolved

tjdiamandis and others added 3 commits December 25, 2020 17:10

Update src/projections.jl -- code review

f7688e1

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

fix types -- code review

9a1f470

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

fix types -- code review

dc2606a

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

matbesancon mentioned this pull request Dec 25, 2020

Switch Random to test-only dependency #33

Closed

matbesancon approved these changes Dec 25, 2020

View reviewed changes

matbesancon reviewed Dec 25, 2020

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

tjdiamandis and others added 3 commits December 25, 2020 17:43

Update src/utils.jl -- code review

b547f28

Co-authored-by: Mathieu Besançon <mathieu.besancon@gmail.com>

_in_exp for grad; fix return type

aee7112

merge

edad4d4

Merge branch 'master' of github.com:tjdiamandis/MathOptSetDistances.jl into master

tjdiamandis marked this pull request as ready for review December 25, 2020 23:38

matbesancon merged commit 133212c into matbesancon:master Dec 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exponential Cone #29

Exponential Cone #29

tjdiamandis commented Dec 23, 2020

codecov bot commented Dec 23, 2020 •

edited

Loading

matbesancon commented Dec 23, 2020

joaquimg left a comment

joaquimg Dec 24, 2020

tjdiamandis Dec 24, 2020

matbesancon Dec 25, 2020

joaquimg Dec 25, 2020

matbesancon Dec 25, 2020

tjdiamandis Dec 25, 2020

joaquimg Dec 25, 2020

matbesancon Dec 25, 2020

joaquimg Dec 24, 2020

tjdiamandis Dec 24, 2020

matbesancon Dec 25, 2020

matbesancon Dec 25, 2020

tjdiamandis Dec 25, 2020 •

edited

Loading

matbesancon Dec 25, 2020

tjdiamandis Dec 25, 2020

matbesancon Dec 25, 2020

tjdiamandis Dec 25, 2020

tjdiamandis commented Dec 25, 2020

matbesancon commented Dec 25, 2020

matbesancon commented Dec 25, 2020

matbesancon commented Dec 27, 2020

		@@ -0,0 +1,37 @@

		function _bisection(f, left, right; max_iters=10000, tol=1e-10)

Exponential Cone #29

Exponential Cone #29

Conversation

tjdiamandis commented Dec 23, 2020

codecov bot commented Dec 23, 2020 • edited Loading

Codecov Report

matbesancon commented Dec 23, 2020

joaquimg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjdiamandis Dec 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjdiamandis commented Dec 25, 2020

matbesancon commented Dec 25, 2020

matbesancon commented Dec 25, 2020

matbesancon commented Dec 27, 2020

codecov bot commented Dec 23, 2020 •

edited

Loading

tjdiamandis Dec 25, 2020 •

edited

Loading