Nondescent direction without warnings

[mateuszbaran]

The warnings are quite annoying in the hyperparameter optimization example.

[codecov[bot]]

Codecov Report

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

Project coverage is 99.73%. Comparing base (20d82e3) to head (d0ba461).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #363 +/- ## ======================================= Coverage 99.73% 99.73% ======================================= Files 73 73 Lines 6805 6822 +17 ======================================= + Hits 6787 6804 +17 Misses 18 18 ```

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

[kellertuer]

That I why I have a few debugs (I think even a general one for messages) which one can set to :Once to at least see if it occurs at all (but then only seeing the first).

I can check whether we can adopt that one here as well.

[mateuszbaran]

We can add a flag to the state that tells whether the warning has been printed but it's still not ideal for my example because there is a lot of optimization calls. Even one warning per optimization is quite a lot.

[kellertuer]

That is why I have encapsulated that in a debug action, and use a bit of the message passing pattern to collect those. As I said, I can check what best to do, but probably only Thursday evening (busy teaching week).

[mateuszbaran]

This check is not a debug thing technically so probably there is not much to improve here. Anyway, this can wait.

[kellertuer]

Well, We also have WarnDebugs (for example if cost increases), where for some algorithms that helps to set some parameters correctly (when theory says: Chose this correct and cost will always decrease, but you can not directly check what correct means or you are too lazy).

Example: Constant Stepsize Gradient Descent has by default such a warning.

[mateuszbaran]

This is not really a matter of choosing parameters correctly; just because you get non-descent directions doesn't mean you're doing something wrong. I'd suggest keeping this check always on unless you're sure your combination of manifold, retraction and vector transport is known to not have this issue.

[kellertuer]

What I do not like is that keyword parameters get a bit crowded by now here.

[mateuszbaran]

It's not too much IMO but maybe they could be better organized? Anyway, this is a useful feature.

[kellertuer]

Sure. I do not say we should not do this. I would still prefer a DebugWarnIfNonDescentDirection or so. But as I said, this week is a bit busy, I can check for that later this week only.

[kellertuer]

I just started with a DebugWarnIfNoDescentDirection – which is quite canonical to do (maybe even a bit repetitive). But since the reset als resets the direction, in that case this would (before or after said iteration where that happens) never “see” that due to the reset.

We can go for the generic DebugMessage instead, which displays any debug message some methods encounter. I will prepare that real quick and you can let me know what you think. And yes these specific ones (only running between steps) and the generic one (collecting messages) could maybe be unified a bit somewhen. This is moralise a grown scheme ;)

[kellertuer]

Now the result is stored in a message internally.

This is only used/set if we do not use :ignore, the message is appended by the reset note if one activates that. The whole thing is printed with the DebugMessage debug, which I enhanced a bit to also have the mode most (newer) messages have, that you can set them up to display only :Once that is for the first occurrence of a message.

test coverage might not yet be given, but I wanted to wait for what you think about the idea. I hopefully adapted the documentation accordingly, already.

[mateuszbaran]

That looks fine. My only suggestion is that maybe the state could just store the inner product v instead of the message as a string? The string could be rendered in get_message(qns::QuasiNewtonState).

[kellertuer]

While I think the storage would not make that much of a difference, a disadvantage might be, that the current message also reports when the direction is reset, so without ignore (but also without the reset) just a warning would be issued but no reset performed. This might be a bit complicated to safe in just a value?

[mateuszbaran]

Whether reset was performed or not could be inferred from qns.nondescent_direction_behavior.

[kellertuer]

Oh, indeed. What would be the advantage of only twiddling together the string in get_message?

[mateuszbaran]

Putting strings together causes allocations and takes some time, that's the main problem. It's fine in debug or reporting code but I prefer to avoid it in the main computational code.

[kellertuer]

Sure, that sounds reasonable. Then the string is only concatenated / interpolated when it is actually needed. We should do that. I can check that tomorrow unless you are faster ;)

[kellertuer]

I can do the test coverage for the get_message and the updated DebugMessages later today, just the test to trigger the reset – for that I do not have an idea yet.

[mateuszbaran]

OK, thanks. I think my change should make the qN part of the code covered.

[kellertuer]

From my side this is fine and could be merged, but since the last commit is mine, it would be great if you could confirm shortly as well that this is fine with you :)

[mateuszbaran]

Sure, everything looks fine.