JuliaManifolds / Manopt.jl

🏔️Manopt. jl – Optimization on Manifolds in Julia
http://manoptjl.org
Other
314 stars 40 forks source link

Refactor get_reason #395

Closed kellertuer closed 2 months ago

kellertuer commented 2 months ago

This resolves #389. I had a bit of a boring train ride today and worked through all stopping criteria, removed .reason fields and when necessary introduced values to store obtained values being printed when generating the string.

There are two small things left

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.77%. Comparing base (4ec57f5) to head (ce56f47).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #395 +/- ## ======================================= Coverage 99.77% 99.77% ======================================= Files 72 72 Lines 7491 7586 +95 ======================================= + Hits 7474 7569 +95 Misses 17 17 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mateuszbaran commented 2 months ago
  • for the Lagrange Multiplier criterion in the bundle methods the symbols should maybe be strings since they might also be -\xi (and not just \xi like now)

That can be saved in a separate field in the criterion but leaving the string in this one place would be fine too.

kellertuer commented 2 months ago

Oh, I missed one solver, in CMA-ES, which has quite a few stopping criteria, they always returned a reason even if they were not active.

For now I think I did rewrite them, but testing them is still missing. We might take the chance and change the is_active fields in these and change them to at_iteration in order to provide the iteration number in the reason (at which the stopping criterion indicated to stop).

mateuszbaran commented 2 months ago

We might take the chance and change the is_active fields in these and change them to at_iteration in order to provide the iteration number in the reason (at which the stopping criterion indicated to stop).

Yes, I think changing it to at_iteration would make sense.

kellertuer commented 2 months ago

We might take the chance and change the is_active fields in these and change them to at_iteration in order to provide the iteration number in the reason (at which the stopping criterion indicated to stop).

Yes, I think changing it to at_iteration would make sense.

Did that. Also tried to use the same tests for get_reason as for others and noticed that on construction, the StopWhenEvolutionStagnatess show method errors (not affecting tests for now, but on REPL it did); since one of the means to compute is (on construction) over an empty array.

kellertuer commented 2 months ago

Hopefully also fixed the last error I mentioned above, which was mainly that the median of an empty vector does not exist, so I introduced a check and tested that as well.