Nicer Interfaces for `::Number` Manifolds and Objectives #236

kellertuer · 2023-04-10T08:52:52Z

Trying to resolve #223 and #235

Todo

…ng solver calls.

Still assert type problems, where I have no clue why some area still thinks we should work with floats in the assertion of types.

kellertuer · 2023-04-10T14:23:59Z

Ah, I at least see the problem.

The NonMonotoneLineSearch uses a storage for a point and a vector, but since it (during initialisation) never “sees” an actual point (i.e. how it is represented) it takes a random point, which is a float.
The storage worked fine with Floats by the way before (I was not even aware of that).

Now since we have a float storage, that of course errors when we com by with 1-arrays. This is not only a problem here but even a general problem if other manifolds use other than their default representation. I did fix the number/vector issue when initialising storage – but will have to check for NMLinesearch. Probably the best is to parr an optional init point and init vector to that as well.

edit:
I think I am one step further, now a problem seems to still be that

copy(Circle(), [0.1]) works fine
copy(Circle(), 0.1) errors since 0.1 is not a Vector{Float64} and of float we have

ERROR: MethodError: no method matching similar(::Float64, ::Type{Float64})
Closest candidates are:
  similar(::Union{LinearAlgebra.Adjoint{T, var"#s886"}, LinearAlgebra.Transpose{T, var"#s886"}} where {T, var"#s886"<:(AbstractVector)}, ::Type{T}) where T at ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/LinearAlgebra/src/adjtrans.jl:207
  similar(::Union{LinearAlgebra.Adjoint{T, S}, LinearAlgebra.Transpose{T, S}} where {T, S}, ::Type{T}) where T at ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/LinearAlgebra/src/adjtrans.jl:211
  similar(::Union{LinearAlgebra.Adjoint{T, S}, LinearAlgebra.Transpose{T, S}} where {T, S}, ::Type{T}, ::Tuple{Vararg{Int64, N}}) where {T, N} at ~/.julia/juliaup/julia-1.8.5+0.aarch64.apple.darwin14/share/julia/stdlib/v1.8/LinearAlgebra/src/adjtrans.jl:212
  ...
Stacktrace:
 [1] allocate(a::Float64, T::Type)
   @ ManifoldsBase ~/Repositories/Julia/ManifoldsBase.jl/src/ManifoldsBase.jl:65
[.... and a long stack trace ...]

So maybe parts of Manifolds are also not so much tested with `::Number``s.

…rach, since we do not have a point to infere the type.

kellertuer · 2023-04-10T17:12:06Z

While this turns out far more complex than I thought, I found out some interesting things.
defining copy as above (for now only in weak dependency in Manopt, maybe later in Manifolds)

I noticed

julia> 1.0 .* inverse_retract(Circle(), fill(0.0), fill(0.0))
0.0

julia> 1.0 * inverse_retract(Circle(), fill(0.0), fill(0.0))
0-dimensional Array{Float64, 0}:
0.0

So I am not so sure I want to use .* any longer to scale tangent vectors?
The same for .-

julia> 1.0 * inverse_retract(Circle(), fill(0.0), fill(0.0)) .- inverse_retract(Circle(), fill(0.0), fill(0.0))
0.0

julia> 1.0 * inverse_retract(Circle(), fill(0.0), fill(0.0)) - inverse_retract(Circle(), fill(0.0), fill(0.0))
0-dimensional Array{Float64, 0}:
0.0

since for now this is what we used in a lot of places but it seems to change the type? Or in short

julia> fill(0.0) .- fill(0.0)
0.0

julia> fill(0.0) - fill(0.0)
0-dimensional Array{Float64, 0}:
0.0

edit: So as soon as we scale vectors using * (and not .* and we subtract vectors using - and not .- (currently just affected Nesterov) – tests are running again. I think I will ask the fill thing the discourse.

codecov · 2023-04-10T17:44:11Z

Codecov Report

Merging #236 (39d3a23) into master (2a5a62e) will decrease coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
- Coverage   99.85%   99.69%   -0.17%     
==========================================
  Files          65       64       -1     
  Lines        5558     5819     +261     
==========================================
+ Hits         5550     5801     +251     
- Misses          8       18      +10

Impacted Files	Coverage Δ
src/Manopt.jl	`100.00% <ø> (ø)`
src/plans/augmented_lagrangian_plan.jl	`100.00% <ø> (ø)`
src/plans/constrained_plan.jl	`100.00% <ø> (ø)`
src/plans/solver_state.jl	`100.00% <ø> (ø)`
src/plans/stepsize.jl	`95.85% <ø> (-4.15%)`	⬇️
src/plans/cache.jl	`100.00% <100.00%> (ø)`
src/plans/gradient_plan.jl	`100.00% <100.00%> (ø)`
src/plans/hessian_plan.jl	`100.00% <100.00%> (ø)`
src/plans/nonlinear_least_squares_plan.jl	`100.00% <100.00%> (ø)`
src/plans/objective.jl	`100.00% <100.00%> (ø)`
... and 18 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…nsistent.

kellertuer · 2023-04-10T18:04:29Z

The switch to 1-dim arrays (using [p] instead of fill(p); we could also use fill(p,1) I think) makes this more reliable (might even revert the changes in Nesterov)

See JuliaLang/julia#28866 thanks to the super-fast answer in https://discourse.julialang.org/t/broadcast-strange-on-0-dimensional-array/97311/2

mateuszbaran · 2023-04-10T18:46:21Z

copy(Circle(), 0.1) errors since 0.1 is not a Vector{Float64} and of float we have

Hm, copy(Circle(), 0.1) should just return 0.1. This should be fixed in ManifoldsBase.jl. I will make a patch.

So maybe parts of Manifolds are also not so much tested with `::Number``s.

Yes, I very rarely use plain numbers in Manifolds.jl.

So I am not so sure I want to use .* any longer to scale tangent vectors?
The same for .-

Oh, I didn't know about that problem with 0-dim arrays. Let's just use one-element 1-dim arrays with broadcasting then.

kellertuer · 2023-04-10T18:55:42Z

copy(Circle(), 0.1) errors since 0.1 is not a Vector{Float64} and of float we have

Hm, copy(Circle(), 0.1) should just return 0.1. This should be fixed in ManifoldsBase.jl. I will make a patch.

great!

So maybe parts of Manifolds are also not so much tested with `::Number``s.

Yes, I very rarely use plain numbers in Manifolds.jl.

Maybe this PR will check a few of those then :) but I think the copy might be the main thing.

So I am not so sure I want to use .* any longer to scale tangent vectors?
The same for .-

Oh, I didn't know about that problem with 0-dim arrays. Let's just use one-element 1-dim arrays with broadcasting then.

Well, t was interesting to learn, and we just use 1-dims and then it should be fine :)

# Conflicts: # Project.toml # src/plans/nonmutating_manifolds_plans.jl

kellertuer · 2023-04-15T08:19:11Z

This is a bit more work than I thought, but a nice occasion to unify and test all high-level interfaces. ALM took me about an hour, but the kwargs got nicer in quite a few places.

mateuszbaran · 2023-04-15T09:24:48Z

Yes, it indeed looks like quite a bit of work but I think it's a good change 🙂 .

kellertuer · 2023-04-15T12:12:49Z

Yes, I think both things are good ideas, both the preprocessing for numbers (though it takes a bit with adopting functions),
and I like the interfaces alg(M, obj, p) if you have an objective that can be run on different manifolds. And all changes are non-breaking indeed.

So this is definetly useful. Just also takes bit to rework.

test/solvers/test_quasi_Newton.jl

kellertuer · 2023-04-25T13:45:56Z

...phew just 10 Algorithms left. It is nice to also review a few tests and improve them, but going through all algorithms is indeed a bit of work by now.

But. Both new things are really nice to have (though I will not do the number thing for the more-involved algorithms like RCPA and PDSSN).

kellertuer · 2023-04-28T14:35:21Z

For EPM it even made phrasing the defaults much nicer, because some required the objective already. So this rework not only separates some code nicely, it also makes some code really nice.

kellertuer · 2023-04-29T06:44:33Z

Checked Chambolle-Pock and PDSSN today, they make not so much sense in this new form, since they have both a lot of mandatory functions (which sure could be wrapped in their objectives) but also quite a few initial mandatory steps, so that the interface with the objective would still be too crowded. Will think about something nicer for them in some next PR and removed them from the list here.

…ctives.

kellertuer · 2023-04-29T08:20:13Z

@mateuszbaran I have one small questions on LevenbergMarquardt

Do you think a variant on Numbers (Circle/PositiveNumbers/Euclidan) would be useful? I am not sure how to test this
expect_zero_residual was not documented, I already added a line, can you add an explanation, what it does? I mean I know what it indicates, but now how that changes the algorithm.

mateuszbaran · 2023-04-29T10:29:14Z

Do you think a variant on Numbers (Circle/PositiveNumbers/Euclidan) would be useful? I am not sure how to test this

That would be a valid optimization problem but I would expect that there should be a better specialized algorithm for that case.

expect_zero_residual was not documented, I already added a line, can you add an explanation, what it does? I mean I know what it indicates, but now how that changes the algorithm.

I don't really have a good explanation how the algorithm is changed. BTW, expect_zero_residual is documented for LevenbergMarquardtState.

kellertuer · 2023-04-29T10:58:22Z

Do you think a variant on Numbers (Circle/PositiveNumbers/Euclidan) would be useful? I am not sure how to test this

That would be a valid optimization problem but I would expect that there should be a better specialized algorithm for that case.

Then I will not cast the special case here

expect_zero_residual was not documented, I already added a line, can you add an explanation, what it does? I mean I know what it indicates, but now how that changes the algorithm.

I don't really have a good explanation how the algorithm is changed. BTW, expect_zero_residual is documented for LevenbergMarquardtState.

Ah, I took that one over then for now, though it does not help much for a user to read that text ;)

kellertuer added 3 commits April 10, 2023 10:46

remove old boilerplate code, sketch new scheme to work with nonmutati…

a93f9e1

…ng solver calls.

Update readme slightly.

429842f

Maybe fix storage?

e1d9f29

Still assert type problems, where I have no clue why some area still thinks we should work with floats in the assertion of types.

kellertuer added 2 commits April 10, 2023 17:05

Switch to (by default) only using the inefficient storage in NMLineSe…

a52b25e

…rach, since we do not have a point to infere the type.

Simplify code, introduce copy.

a1c094a

get tests back up running again.

6c0c8b8

switching from 0-dim to 1-dim arrays, where .+ and .* are more co…

bd02e10

…nsistent.

kellertuer changed the title ~~remove old boilerplate code, sketch new scheme to work with nonmutati…~~ Nicer Interfaces for ::Number Manifolds and Objectives Apr 10, 2023

mateuszbaran mentioned this pull request Apr 10, 2023

Allow copying scalars JuliaManifolds/ManifoldsBase.jl#154

Merged

kellertuer added 3 commits April 11, 2023 07:57

bump dependencies.

2d9ad3e

Merge branch 'master' into kellertuer/more-interfaces

131ae75

# Conflicts: # Project.toml # src/plans/nonmutating_manifolds_plans.jl

refactor ALM.

46d1ba5

kellertuer added 8 commits April 16, 2023 15:16

fix a test and increase test coverage.

bec9dfa

add Julia 1.9.0

716870a

adapt CG.

8669e28

Increase code coverage.

98c27b2

fix a typo.

4579fbc

Merge branch 'master' into kellertuer/more-interfaces

127d50f

Extend quasi Newton.

8dbf8af

improve test format and test coverage.

f662627

github-actions bot reviewed Apr 21, 2023

View reviewed changes

test/solvers/test_quasi_Newton.jl Outdated Show resolved Hide resolved

increase test coverage.

4a192ed

kellertuer added 5 commits April 25, 2023 16:18

increase test coverage.

ef639d3

fixes a typo to also cover the last two lines.

be6caad

rework subgradient.

e6cc0ca

increase code coverage.

40b1895

dcppa, step I.

8b57bfa

mateuszbaran mentioned this pull request Apr 28, 2023

Use GeodesicInterpolationWithinRadius for Circle mean JuliaManifolds/Manifolds.jl#594

Draft

refactor EPM

c6a1239

kellertuer added 3 commits April 28, 2023 17:03

increase code coverage.

9d785c7

update docs.

4b5d077

Rework AGM.

005009c

kellertuer added 3 commits April 29, 2023 09:19

rework FW.

23243df

Reqork LM; fix FW (no random point possible, since it has to be in C).

a53b9f1

Finish reworking LM and already introduce a method to undecorate obje…

1e934bb

…ctives.

kellertuer added 3 commits April 29, 2023 10:51

rename jacB to jacobian_tangent_basis, depreacting jacB for now.

00bec98

rework particle swarm.

eb5ed6e

rework particle Swarm.

1a6b7e4

Finish tests, check docs.

35cb974

kellertuer marked this pull request as ready for review April 29, 2023 12:39

kellertuer mentioned this pull request Apr 29, 2023

Implement a fill(p) prequel for every solver, where p might be a number. #235

Closed

kellertuer added 2 commits April 29, 2023 14:41

bump version.

862922d

increase code coverage.

39d3a23

kellertuer merged commit 34d1bf4 into master Apr 29, 2023
12 of 13 checks passed

kellertuer deleted the kellertuer/more-interfaces branch December 16, 2023 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nicer Interfaces for `::Number` Manifolds and Objectives #236

Nicer Interfaces for `::Number` Manifolds and Objectives #236

kellertuer commented Apr 10, 2023 •

edited

kellertuer commented Apr 10, 2023 •

edited

kellertuer commented Apr 10, 2023 •

edited

codecov bot commented Apr 10, 2023 •

edited

kellertuer commented Apr 10, 2023

mateuszbaran commented Apr 10, 2023

kellertuer commented Apr 10, 2023

kellertuer commented Apr 15, 2023

mateuszbaran commented Apr 15, 2023

kellertuer commented Apr 15, 2023

kellertuer commented Apr 25, 2023

kellertuer commented Apr 28, 2023

kellertuer commented Apr 29, 2023

kellertuer commented Apr 29, 2023

mateuszbaran commented Apr 29, 2023

kellertuer commented Apr 29, 2023 •

edited

Nicer Interfaces for ::Number Manifolds and Objectives #236

Nicer Interfaces for ::Number Manifolds and Objectives #236

Conversation

kellertuer commented Apr 10, 2023 • edited

Todo

kellertuer commented Apr 10, 2023 • edited

kellertuer commented Apr 10, 2023 • edited

codecov bot commented Apr 10, 2023 • edited

Codecov Report

kellertuer commented Apr 10, 2023

mateuszbaran commented Apr 10, 2023

kellertuer commented Apr 10, 2023

kellertuer commented Apr 15, 2023

mateuszbaran commented Apr 15, 2023

kellertuer commented Apr 15, 2023

kellertuer commented Apr 25, 2023

kellertuer commented Apr 28, 2023

kellertuer commented Apr 29, 2023

kellertuer commented Apr 29, 2023

mateuszbaran commented Apr 29, 2023

kellertuer commented Apr 29, 2023 • edited

Nicer Interfaces for `::Number` Manifolds and Objectives #236

Nicer Interfaces for `::Number` Manifolds and Objectives #236

kellertuer commented Apr 10, 2023 •

edited

kellertuer commented Apr 10, 2023 •

edited

kellertuer commented Apr 10, 2023 •

edited

codecov bot commented Apr 10, 2023 •

edited

kellertuer commented Apr 29, 2023 •

edited