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

Change rrule verbosity behaviour #106

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Change rrule verbosity behaviour #106

wants to merge 2 commits into from

Conversation

Jutho
Copy link
Owner

@Jutho Jutho commented Nov 14, 2024

This drastically changes the behaviour of verbosity in the rrules:

  • The warnings printed by the rrules are now controlled by alg_primal.verbosity rather than alg_rrule.verbosity. This way, alg_rrule is fully restricted to specifying the linear/eigen subproblem solved in the rrule. I think it is more naturally that warnings or info printed in the rrule of a certain algorithm are controlled by the verbosity setting of that algorithm
  • The required verbosity level for all warnings in the rrules is increased by one, i.e. with verbosity=0 none of the warnings will be printed. I am still not sure if that is the right decision. Some of these warnings could point to errors. Whereas for the primal algorithm, it is the user's responsibility to check for convergence in the info struct, so that it makes sense that warnings about lack of convergence are only printed for verbosity >= 1, it is impossible to check this for the pullback
  • Some tolerances are also increased a bit to have warnings triggered less quickly overall

Aside from this, this PR also removes MinimalVec and takes the one from VectorInterface.jl 0.5. Maybe that required a separate PR, but here we are 😸 .

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.94%. Comparing base (4d2a06f) to head (b2c0f81).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   85.87%   85.94%   +0.06%     
==========================================
  Files          30       30              
  Lines        3286     3286              
==========================================
+ Hits         2822     2824       +2     
+ Misses        464      462       -2     

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

@lkdvos
Copy link
Collaborator

lkdvos commented Nov 14, 2024

What happens if you just add a -1 setting to disable all warnings, and still have it default to printing the warnings as well?

@Jutho
Copy link
Owner Author

Jutho commented Nov 14, 2024

Then you get the old behaviour if I understand your suggestion correctly (except then for the alg_reverse.verbosity -> alg_primal.verbosity change).

@lkdvos
Copy link
Collaborator

lkdvos commented Nov 14, 2024

I am okay with not printing the warnings and having them on an opt-in level, it's just that I don't think I realized I could turn them off by setting the verbosity to -1, and maybe other people also didn't. What I mean then is, that it might not be necessary to change this behavior, if you could just get it to behave like that anyways with a setting.

Ultimately though, I have no strong feelings about this, I guess that if things are going wrong I would typically start debugging by setting the verbosity levels higher anyways.

@Jutho
Copy link
Owner Author

Jutho commented Nov 14, 2024

The verbosity = -1 level was used in the tests, since, if you compute full Jacobians, you are bound to probe gauge-dependent directions. But indeed, nobody was probably aware of this and it was a hacky solution, so I guess the current one is better. Alternatively, I could also keep at least the actual convergence warnings at level zero, but mention explicitly that level -1 could be used to disable them.

@Jutho
Copy link
Owner Author

Jutho commented Nov 15, 2024

Thinking some more about it, current behavior for the forward algorithms is that

  • at verbosity=0 they do not print anything
  • at verbosity=1 they print a warning if the number of requested solutions is obtained (with convergence up to specified tolerance), but also an info message if the number of solutions is found, with details of number of iterations / ...
  • at verbosity=2 you get per iteration info

I could remove the info message at verbosity = 1. Then both the forward algorithm and the reverse rule have "verbosity = 1" being something like "warning mode". No info, only warnings. The question is then if having only the final info requires a separate level (level 2), and so the per iteration info shifts to level 3.

@XingyuZhang2018
Copy link
Contributor

I think an additional level should be included:

  • At verbosity=0, no output is printed.
  • At verbosity=1, a warning is printed.
  • At verbosity=2, you receive start and end information.
  • At verbosity=3, you receive information for each iteration.

This addition is necessary because there could be numerous iterations that might overwhelm the screen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants