An idea to provide Hints and Errors on Keywords

[kellertuer]

Since a few things are nested and some keywords are “passed on”, e.g. to the debug or record factory,
currently there is no errors or warnings when a keyword is provided, that says “unused”.

There is also no easy way to tell which keywords could be passed e.g. to a certain solver,

This is a small experiment based on the approach from Makie.jl to see whether we can warn, error and/or give hints on keywords. Similarly, this should be usable to upfront determine all keywords that are accepted by a function … in the long run maybe even with a remark what/where it would be used internally for

🗺️

• test with one state and solver how to best create their list of keyword list
• find a way to verify existing keywords
• implement a difference to find unused ones
• warn or even error (a strict mode?) on unused keywords
• export the function that returns all keywords
• implement a Levenstein distance to find closest keywords if someone has a typo?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.79%. Comparing base (a91e67a) to head (87188f4).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master     #503    +/-   ##
========================================
  Coverage   99.78%   99.79%            
========================================
  Files          85       86     +1     
  Lines        9418     9640   +222     
========================================
+ Hits         9398     9620   +222     
  Misses         20       20            

☔ 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]

A first small thing that works by now – even using two macros to define keywords for a single function and one collecting all it passes on is that a first function can now report its keywords and where they are passed on to in the end.

Julia> Manopt.accepted_keywords(gradient_descent)
Keywords for gradient_descent:

accepted: 
  * cache (passed on to decorate_objective!)
  * callback (passed on to decorate_state!)
  * count (passed on to decorate_objective!)
  * debug
  * differential
  * direction
  * evaluation
  * objective_type (passed on to decorate_objective!)
  * p (passed on to decorate_objective!)
  * record (passed on to decorate_state!)
  * retraction_method
  * return_objective (passed on to decorate_objective!)
  * return_state (passed on to decorate_state!)
  * stepsize
  * stopping_criterion
  * X

[mateuszbaran]

That's an interesting idea, though I would expect there to be an upstream library for that? So that each library that wants to do that doesn't make a separate function for keyword hints. For example REPL could offer keyword autocomplete based on such information in the future.

BTW, I don't think extract_keywords needs to be a macro. You should be able to go like this:

julia> f(; a=10) = a
f (generic function with 1 method)

julia> m1 = methods(f)[1]
f(; a)
     @ Main REPL[30]:1

julia> Base.kwarg_decl(m1)
1-element Vector{Symbol}:
 :a

This would avoid interfering in function definitions.

[kellertuer]

That is an awesome remark, since I was already thinking about how to combine methods of one function in the macro. nice was to avoid that!
That will simplify that a lot. I can still filter some internal keywords, so that is nice.

I think the main difference to classical keywords and their autocomplete is this nested-and-pass-on-kezwords.

[kellertuer]

The other macro is also not necessary, one can just define a

function_passes_kws_to(::typeof(gradient_descent) = [gradient_descent!,]

and use that in the accepted keywords function.
Nice! Will continue this when I have finished my ideas on the semidirect rework.

[kellertuer]

Gave it a 5-minute try that took me 12,
and besides the fact that Base.kwarg_decl(methods(decorate_objective!)[1]) currently returns twice also a :P

10-element Vector{Symbol}:
 :P
 :P
 :cache
 :count
 :objective_type
 :p
 :_embedded_p
 :_embedded_X
 :return_objective
 Symbol("kwargs...")

which might also just be me having defined that a bit strange in the keywords (have to check) – one can filter the kwargs... and it works very well and without all the macros.

It was still nice to learn a bit about macros, though I could now – with your idea for the one and mine for the other – omit both macros completely. Ok, the function you proposed is internal so it might break, but the code is really much much simpler now. Thanks for the idea – I will carefully check for the :P – but I can basically already continue to a check next :)

[kellertuer]

This now indeed works. I still have to go though solvers and maybe some constructors to see where they pass kwargs... to and set the calls_with_kwargs(f) = Tuple... for those functions correctly.

in mode=:error one can already get (for solvers until CMAES&grad_desc):

julia> gradient_descent(M, f, grad_f; fnump=1)
ERROR: gradient_descent! does not accept the keyword(s)

  * fnump

Hint: gradient_descent! does accept the following keywords:

  X, cache, callback, count, debug, differential, direction, evaluation, objective_type, record, retraction_method, return_objective, return_state, stepsize, stopping_criterion

And the mode (whether to warn or to error) I maybe want to set as a preference like the tutorial mode.

So this small experiment is on a good way.

[mateuszbaran]

That looks good 👍 . One minor note, I'd suggest changing the name in to something else in Keywords struct. in is already a function name in base Julia.

[kellertuer]

good point, maybe something like accepted_in?

[kellertuer]

it actually states the “current function” the keywords are meant for, so something along/similar to function (again not directly that ;)) would be fitting as well.

Ah! from would fit because it might even indicate in a longer sense where a kw is from.

[mateuszbaran]

I think both accepted_in and from would be fine.

[kellertuer]

This is nearly done. It is already working fine.

• make sure test coverage is there
• maybe implement a hint when a very similar keyword exists, cf https://github.com/MakieOrg/Makie.jl/blob/d8f53444bc7d328d351d3265b1380d279eb2058b/Makie/src/recipes.jl#L740-L821 (but I have to check how large the effort for this is)

The good thing: With this I found one small bug where we passed on an unnecessary keyword (evaluation when it was no longer accepted) and found two typos of keywords in the tests. Yay!

[kellertuer]

I checked the code that Makie uses for hints and it looks too involved for me to just take over. So I leave hints for a next PR and declare this PR finished.

[mateuszbaran]

Everything else looks fine to me, so it's just the origins thing that I'd suggest improving.