Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 20 commits into from

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Jan 31, 2024

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

  • check which other solvers benefit from the “Tutorial” and the new warning
  • check test coverage
  • document the new mode
  • illustrate / mention the new mode in the intro tutorial.

@kellertuer kellertuer changed the title Add a persistent layer of setting options and introduce a “Tutorial” mode Add a persistent layer of setting options and introduce a Tutorial mode Jan 31, 2024
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kellertuer
Copy link
Member Author

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 kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Feb 2, 2024
Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

src/plans/debug.jl Outdated Show resolved Hide resolved
src/plans/plan.jl Outdated Show resolved Hide resolved
src/plans/plan.jl Outdated Show resolved Hide resolved
src/plans/plan.jl Outdated Show resolved Hide resolved
src/plans/plan.jl Outdated Show resolved Hide resolved
src/plans/plan.jl Outdated Show resolved Hide resolved
src/plans/plan.jl Outdated Show resolved Hide resolved
src/plans/stopping_criterion.jl Outdated Show resolved Hide resolved
src/solvers/gradient_descent.jl Outdated Show resolved Hide resolved
tutorials/Optimize.qmd Outdated Show resolved Hide resolved
kellertuer and others added 2 commits February 2, 2024 20:31
Co-authored-by: Mateusz Baran <[email protected]>
@kellertuer
Copy link
Member Author

kellertuer commented Feb 2, 2024

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
Copy link
Member

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

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few last minor things.

docs/src/plans/index.md Outdated Show resolved Hide resolved
src/plans/debug.jl Outdated Show resolved Hide resolved
tutorials/Optimize.qmd Outdated Show resolved Hide resolved
src/plans/plan.jl Outdated Show resolved Hide resolved
@kellertuer
Copy link
Member Author

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

@kellertuer kellertuer closed this Feb 5, 2024
@kellertuer kellertuer reopened this Feb 5, 2024
@kellertuer kellertuer closed this Feb 5, 2024
@mateuszbaran
Copy link
Member

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

@mateuszbaran
Copy link
Member

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

@kellertuer
Copy link
Member Author

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.

mateuszbaran added a commit that referenced this pull request Feb 5, 2024
…de (#354)

* Add Preferences for persistent global settings.
* Document the `:Mode` setting and implement a reset possibility
* Add the new global code to Quasi Newton Debug and the start tutorial.
* Update Changelog.

---------

Co-authored-by: Mateusz Baran <[email protected]>
@kellertuer kellertuer deleted the kellertuer/global_mode branch May 4, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants