Closed tjdiamandis closed 3 years ago
Merging #29 (edad4d4) into master (3af4edb) will increase coverage by
1.10%
. The diff coverage is93.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%) |
:arrow_down: |
src/utils.jl | 94.11% <94.11%> (ø) |
|
src/projections.jl | 98.78% <96.29%> (-1.22%) |
:arrow_down: |
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.
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)
Julia 1.0 test failing due to a new version of diagm
being used. Is the "correct" way to construct a diagonal matrix from a vector Diagonal(v)
?
Yes Diagonal(v) works. Usually it should be preferred when you will not add non-diagonal entrie to the matrix afterwards, because it creates a special matrix type:
julia> diagm([1,2])
2×2 Array{Int64,2}:
1 0
0 2
julia> Diagonal([1,2])
2×2 Diagonal{Int64,Array{Int64,1}}:
1 ⋅
⋅ 2
Sorry for the type return errors. Thanks so much for the thorough review! I understand the general design principle now.
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
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!
Thanks again for the PR!
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.