JuliaManifolds / Manopt.jl

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

Add a persistent layer of setting options and introduce a Tutorial mode #354

Closed kellertuer closed 6 months ago

kellertuer commented 7 months ago

This adresses a point raised in #346 and is for now realised in the quasi_Newton solver.

codecov[bot] commented 7 months ago

Codecov Report

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

Comparison is base (8851619) 99.45% compared to head (29f8402) 99.53%. Report is 2 commits behind head on master.

:exclamation: Current head 29f8402 differs from pull request most recent head ed5739b. Consider uploading reports for the commit ed5739b to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #354 +/- ## ========================================== + Coverage 99.45% 99.53% +0.08% ========================================== Files 69 69 Lines 6418 6454 +36 ========================================== + Hits 6383 6424 +41 + Misses 35 30 -5 ```

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

kellertuer commented 7 months ago

So the tutorial already has a section about this, I just have to figure out how to convince Quarto to display warnings :)

https://manoptjl.org/previews/PR354/tutorials/Optimize/#Using-the-“Tutorial”-mode

kellertuer commented 7 months ago

The suggested usage of Preferences.jl is that you restart your Julia session to take any preference changes into account. This ensures that all preference checks can be done very fast. Are you sure you want dynamic checks here?

My honest answer is: I would prefer to be able to set this dynamically – otherwise I for example have no clue how to test that? How do I restart Julia in testing to test the other mode? For now this is persistent after starts, yes. But dynamic.

Besides the testing, I would also have not much clue how to implement what you refer to? So one would set the preference but that is never accessed but something different, because only on start (load of Manopt) the local preferences of preferences.jl are read? To me (but maybe I am just not that clever here) that reads like probably 100 more lines of code to split that into two layers, one that we load and one that we have during runtime?

edit: I also do not fully agree with your statement, the first word here is the important one I think:

When your package sets a compile-time preference, it is usually best to suggest to the user that they should restart Julia, to allow recompilation to occur.

so with the macros it can be used at compile time, it does not say it has to, and the way I have this feature in mind here, I would not know how this should be done at compile time for the tutorial mode.

mateuszbaran commented 7 months ago

I thought about it again and you are most likely right to keep preferences dynamic. Let's not over-complicate it.

kellertuer commented 6 months ago

GitHub has some Hickups today, first not being able to pull from gh-pages, now this is merged but this PR not closed?! Strange.

mateuszbaran commented 6 months ago

I don't see this as merged, only closed.

mateuszbaran commented 6 months ago

Wait, master branch history shows it's merged but his PR does not. Weird.

kellertuer commented 6 months ago

I see a commit that refers to this on master, but also this not marked as merged. But if I open it, the changelog check fails, since the changelog is identical with master – so it is on master. I am confused.