Print debug also in initial iteration

[JoshuaLampert]

Addresses the discussion started at #544 (comment). I went through the different debug options and enabled printing at the initial value for those I evaluated it would make sense to print the debug statement at the first iteration. If anything is missing or you don't think it makes sense to print it already at the initial value, I am open to adapt that. The only one I would like to keep is the feasibility one.

[JoshuaLampert]

For example I am not sure about this:

Manopt.jl/src/plans/proximal_plan.jl

Line 284 in 7373e5b

(k > 0) && Printf.format(d.io, Printf.Format(d.format), cpps.λ(k))

and this:

Manopt.jl/src/plans/primal_dual_plan.jl

Line 735 in 7373e5b

if all(has_storage.(Ref(d.storage), [:Iterate, :X, :n])) && k > 0 # all values stored

and the other ones in primal_dual_plan.jl

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7373e5b) to head (cd77b0d).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #552   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           91        91           
  Lines         9976      9975    -1     
=========================================
- Hits          9976      9975    -1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kellertuer]

Thanks!

A small problem might be consistency. I do not mean to prior prints – to me it is fine to “break” old code in this sense, since it just breaks what is seen in the print.

Until now only the cost was printed at the beginning, now you chose a slightly larger set. The two you mentioned, I think could be printed. Furthermore:

• What about the initial point?
• ...or all others in
  

  Manopt.jl/src/plans/debug.jl

  Lines 1401 to 1410 in 7373e5b

  	(d == :Cost) && return DebugCost()
  	(d == :Change) && return DebugChange()
  	(d == :Gradient) && return DebugGradient()
  	(d == :GradientChange) && return DebugGradientChange()
  	(d == :GradientNorm) && return DebugGradientNorm()
  	(d == :Iterate) && return DebugIterate()
  	(d == :Iteration) && return DebugIteration()
  	(d == :Feasibility) && return DebugFeasibility()
  	(d == :Stepsize) && return DebugStepsize()
  	(d == :Stop) && return DebugStoppingCriterion()

(i.e. excluding the warnings and times)?

I do not have an answer yet, but we could either

• check to choose a new good default
• ...additionally allow to set that? The iterate might (for most manifolds) be too long)

One simple idea, which however expands on complexity of using these symbols would be tuples of symbol and bool (or even a pair?) (:Cost, true) or (:Cost, init => true) (we could even remodel to pass pair as kwargs...

just a few ideas. If you feel this is too complicated, we stick to just choosing a new default what to print at init.

PS: Cool that you directly adopted mentioning the PR in the changelog :)

[JoshuaLampert]

I'm definitely in favor of consistency. I also like the possibility of customization. Since many of the Debug* already allow keywords, we could add a boolean keyword init to them with sensible default, which can be set by explicitly creating the struct via the constructor. I think I would keep the symbol construction as it is, i.e., it always creates the default. Otherwise it would be inconsistent with how other keyword arguments are handled.
Let's go through the list:
in debug.jl:

• DebugCost: Currently printed at k = 0. I would add init = true as kwarg.
• DebugChange: Currently not printed at k = 0. I think here is doesn't make sense because for the change we already need a previous value. I would keep it as it is.
• DebugDivider: Currently printed at k = 0. I would add init = true as kwarg.
• DebugEntry: Currently printed at k = 0. I would add init = true as kwarg.
• DebugFeasibility: Currently (in master) not printed at k = 0. I would add init = true as kwarg.
• DebugIfEntry: Currently printed at k = 0. I would add init = true as kwarg.
• DebugEntryChange: Similar to DebugChange. I would keep it as is.
• DebugGradientChange. Same again. I would keep it as is.
• DebugIterate: Currently not printed at k = 0. I would add an init = false as kwarg.
• DebugIteration: Already handles k = 0 appropriately. Would keep it as is.
• DebugMessages: Would keep it as is.
• DebugStoppingCriterion: Currently not printed at k = 0. I would guess there will never be a stopping reason before the first iteration started. So I would keep it as is.
• DebugWhenActive: I think we can keep it as it is.
• DebugTime: At k = 0, this initializes the time. So it doesn't make sense to print it there, i.e., I would keep it.
• DebugWarnIfCostIncreases: This will never print at k = 0 because the initial old_cost is Inf. So keep as is.
• DebugWarnIfCostNotFinite: Currently printed at k = 0. I would keep it.
• DebugWarnIfFieldNotFinite: Same as DebugWarnIfCostNotFinite.
• DebugWarnIfGradientNormTooLarge: Same as DebugWarnIfCostNotFinite.
• DebugWarnIfStepsizeCollapsed: Similar to DebugWarnIfCostNotFinite.

So the only one for which I would change the default it DebugFeasibility. Do you have another opinion for one or more of the above? We could then also discuss other Debug* entries located in other files.

[kellertuer]

Thanks for the great overview; sure we can keep the symbols as is, one can always also just pass the full DebugCost(; init=false) instead of :Cost into the debug string, so that should be fine.

From you list all sounds great! I shortly thought about naming and checked with Mateusz. We think init=true might be hard to parse/digest by users, since it might also indicate whether to initialise something in the debug or not

Would maybe at_init = true|false is a better name? Then it reads quite nice Debug(; at_init=true).

With that and your comprehensive, good overview and list – yes great! Let's do that :)

[JoshuaLampert]

Yes, at_init sounds better. I'll add this kwarg to the locations I listed above in a second :)

[JoshuaLampert]

I added at_init as discussed to all the debug options in debug.jl. So let's discuss the other options in other files:
bundle_plan.jl:

• DebugWarnIfLagrangeMultiplierIncreases: Would keep as is because it needs a previous value.

proximal_plan.jl:

• DebugProximalParameter: Currently not printed at k = 0. I would add init = true as kwarg.

primal_dual_plan.jl:

• DebugDualResidual: Can probably stay as it needs storage.
• DebugPrimalResidual: Can probably stay as it needs storage.
• DebugPrimalDualResidual: Can probably stay as it needs storage.
• DebugDualChange: Can probably stay as it needs storage.

first_order_plan.jl:

• DebugGradient: Currently not printed at k = 0. I would add init = true as kwarg.
• DebugGradientNorm: Currently not printed at k = 0. I would add init = true as kwarg.
• DebugStepsize: Currently not printed at k = 0. I think we do not have get_last_stepsize at k = 0? So probably keep it as is?

proximal_bundle_method.jl:

• DebugWarnIfLagrangeMultiplierIncreases: Can probably stay as it needs an old value.

So I would change DebugProximalParameter, DebugGradient, and DebugGradientNorm.

[kellertuer]

Looks good to me, just two small comments:

• for the gradient I would keep at_init = false similar as to DebugIterate, only on very small dimensions it makes sense to print this. But Gradient norm we could do, sure.
• for stepsize the last one is indeed not so useful, but one could actually print the initial one? If not, then it should stay as you write.

[JoshuaLampert]

• Sounds good to me.
• If we would print the initial stepsize at k = 0, the same one would be printed as get_last_stepsize for k = 1, right? This might be confusing. So I would say, let's keep it.

[kellertuer]

I do not think so, because the default debug is done after a run, if I remember correctly, so after the first iteration the steps size should be fine and probably different.

[JoshuaLampert]

Ah, I see. So in order to get the initial step size, would get_last_stepsize for k = 0 already be fine or would we need to get the initial stepsize differently?

[kellertuer]

If I remember correctly the last stepsize at k=0 should return the initial one, yes.

[JoshuaLampert]

Tests pass. So this is ready for a review from my point of view, @kellertuer. Thanks for your input, @kellertuer.

[kellertuer]

Looks good. Thanks again!

[kellertuer]

Oh, the code looks fine but you seem to have broken the tests?

edit: Ah I see; yes, those two tests check what is printed in the initial step and there we now print also the iterate / something. You could check and set their at_init to false or adapt the string we check

[JoshuaLampert]

Yes, I also just saw this. The tests test for the printed output, which has now changed. Should I change the reference string or construct the necessary Debug* with at_init = false or something else?

[JoshuaLampert]

I just realized that the failing tests came from DebugIterate. However, we agreed on setting at_init = false as default for DebugIterate, but for some reason I set it to true by default. So I changed that to false, which hopefully also fixes the test.