Make solver signatures more consistent#113
Conversation
Make the method signatures of quasi_Newton, cyclic_proximal_point,a and ChambollePock consistent with other solver functions such as for example gradient_descent. A generic type instead of `Function` allows the methods to be called with a function like object. This pattern is for example demonstrated in the geodesic regression tutorial.
mateuszbaran
left a comment
mateuszbaran
left a comment
There was a problem hiding this comment.
I think this is a good idea 👍 .
|
I had some problems with the There is also another inconsistency which might be a bit harder to correct:
This makes it difficult to write generic code, but I am not sure if you are willing to possibly break somebodies code to fix this. |
|
Thanks for noticing this. Parts of this package (ChambollePock especially) were written before I was aware of this nice parameter trick.
Besides these the idea is very good, just formatter is currently failing and please make sure all tests. run fine :) |
Codecov Report
@@ Coverage Diff @@
## master #113 +/- ##
=======================================
Coverage 99.63% 99.63%
=======================================
Files 47 47
Lines 3023 3027 +4
=======================================
+ Hits 3012 3016 +4
Misses 11 11
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
Is there a specific version of Because the CI test is complaining something that was adapted by my version of JuliaFormatter: |
|
That's a very old version of JuliaFormatter, I'm currently using 0.22.6. Sometimes (though rarely) the formatter changes how it formats files with the same options and IIRC the CI formatter action always uses the newest version. |
|
Note also that even more importantly here, that you have to run the Julia formatter from the Manopt.jl folder since there is a config file in this folder to chose the style. |
|
I must still be doing something wrong, because for me it keeps changing a bunch of files but for example sticks to the 3-line I am probably again missing something obvious: I tried to update JuliaFormatter by running |
|
You need to run for example |
|
Thanks, I didn't know that. I ran |
|
THen there might be one of your packages constraining it to not go beyond 13.x, maybe do an |
|
If |
|
General But running |
|
Hm, newer versions are not marked as compatible with Atom for some reason: JunoLab/Atom.jl#381 . I'm afraid in such case the only quick solution is making a special Pkg environment just for JuliaFormatter, though ideally someone would just bump compat bounds in Atom. |
|
Here is a short quick script that should make formatting simple for you: julia> using Pkg; Pkg.activate(; temp=true); Pkg.add("JuliaFormatter"); using JuliaFormatter; format(".")Just run it in the folder with Manopt sources 🙂 . |
|
Ah the temp solution is also a good one. We made a short zoom session to learn about reading the package version conflicts and I convinced @const-ae to try VS code :) |
|
Just to check – I had two questions atop that could be addressed here as well. It is no problem not to do them here – but just let me know. If you understand what I meant and want to address them here, I will leave this open until they are – if not we can merge this. |
|
Sorry for the late response, I was at some workshop last week.
I assume you are referring to:
I think these two question are probably better left for another PR, because I won't get to address them any time soon. |
|
Well, nearly – for the second question I would like to hear a yes, since otherwise you would break some code. I feel like it would/should work, but did not yet have the time to check. The first point in your quote is then something for another PR, no problem. |
kellertuer
left a comment
kellertuer
left a comment
There was a problem hiding this comment.
I had finally the chance to check the Stochastic variants, where I feared you might have oversimplified the unification – but! All is fine 👍
We could just bump the version number here. If you want, feel free to already add yourself to the .zenodo file similar to the Manifolds.jl case; this is just a small fix but together with the check (will hopefully look closer at that soon) I think this is already justified :)
Thanks again!
|
Great. I have added myself and bumped the version number :) |
3 participants
Use generic type instead of
Functionfor cost function in some solversMake the method signatures of quasi_Newton, cyclic_proximal_point, and ChambollePock consistent with other solver functions such as for example gradient_descent. A generic type instead of
Functionallows the methods to be called with a function like object. This pattern is for example demonstrated in the geodesic regression tutorial.