Avoid leaky towers of kwargs

[amilsted]

I think it would be nice to remove the leaks from the "kwargs tower" in this package. Lots of functions ultimately pass kwargs down to tdvp(direction::Base.Ordering, solver, PH, time_step::Number, psi::MPS; kwargs...) in tdvp_step.jl, but there is no checking for "leftover" kwargs that are never used.

I want to know if I am supplying a kwarg that isn't used anywhere!

Perhaps one fix is to have explicit keywords in the method signature for that particular function, or alternatively to pop() from kwargs and check it's empty at the end?

PS: Is there a way to pass kwarg docstrings back up the stack to the docstrings of higher-level functions?

[amilsted]

One possible issue: kwargs is also passed to the user-defined checkdone() in tdvp_generic.jl. Perhaps we can provide a separate checkdone_kwargs for that?

[emstoudenmire]

While I fully agree it's good to get an error message if you supply a kwarg that isn't used anywhere, right now this seems like an issue where the design of Julia offers some tradeoffs and hard to make the perfect one.

Currently the code in this package is rather explicit about the kwargs that get passed down to each function (from tdvp, to tdvp_sweeps, to tdvp_step, etc.). One downside, though, is that it's a lot of work to "plumb" a new kwarg through, especially to lower levels like the solver. Some of the kwargs aren't getting passed, so there are now another kind of silent bug.

Yet another downside is that now the code is very long and verbose with lots of explicit kwarg reading and passing everywhere. It's harder to maintain this way I believe.

Unless there is a third choice besides:

1. just passing kwargs... through all the way down
2. explicitly listing and unwrapping and re-passing 10+ kwargs to all the intermediate functions just so they can get passed to the lowest levels

then I would vote we live with #1, and the unfortunate silent failures / lack of error messages that can result from this approach.

Happy to discuss more, because maybe there's a way to balance these considerations that I missed.

[amilsted]

Yeah, the current code is relatively explicit in a few places. I think it's a side-effect of the "process the kwargs in the function body with get() pattern"? I think I prefer multi-line method signatures for getting keyword arguments needed in the present function, combined with bundled passing of kwargs... otherwise.

I'm mainly concerned with documentation and catching unused kwargs. Perhaps the suggestion here to used structs to hold large sets of arguments, is the "right" alternative to using kwargs.

Anyway, to catch unused kwargs, it is enough if the "tower" has a "bottom".

function ftop(a,b; kw1=nothing, kwargs...)
    ...
    fmid(b; kw2=whatever, kwargs...)  # this function does not get to see `kw1`
end

function fmid(b; kw2=nothing, kwargs...)
    ...
    fbottom(c; kw3=something, kwargs...)
end

function fbottom(c; kw3=nothing, kw4=nothing)   # DOES NOT SLURP KWARGS
   return "oh hi"
end

ftop(a,b; kw5="muahaha")   # unused kwarg causes error to be thrown

WIth the "get kwargs in the body" pattern, one could achieve similar by "popping" kwargs rather than "getting" them. Unfortunately, this does not solve the issue with documenting all these arguments and making them discoverable! The struct solution is much better for those things.

[emstoudenmire]

Thanks for the helpful and interesting discussion. That struct approach in the link is an interesting one, and points to how a keyword system, as good as Julia's is, could be even better in principle (and maybe a combination of existing language features plus some new ones in the future could lead to better practices).

I see really well what you mean now about:

• the 'tower' having a base that doesn't slurp
• the idea of "popping" by getting the kwargs in the function definition, and avoiding the get approach. I think I was the one who started using that approach a lot in ITensors.jl but now in hindsight I'm not really sure why I thought it was needed. (There are some corner cases where it can be better, but only pretty rare ones maybe.)

So I think that pair of best practices above goes a long way toward fixing issues with keyword args. Thanks –

[mtfishman]

I agree that I don't like the current way we handle keyword arguments, I believe it is a historical artifact from porting the C++ DMRG code. @amilsted I think the approach you outline is good, and it would be nice to avoid using get if possible.

[mtfishman]

Also I think we should make use of structs/NamedTuples to pass certain groups of keyword arguments that we know will all get passed to one function lower down (such as keyword arguments for the solver).

[emstoudenmire]

Agreed we should think of these as the new “best practices” for this package and others. I’ll start incorporating these patterns into my PRs.

On Tue, Oct 11, 2022 at 6:04 PM Matt Fishman @.***> wrote:

Also I think we should make use of structs/NamedTuples to pass certain groups of keyword arguments that we know will all get passed to one function lower down (such as keyword arguments for the solver).

— Reply to this email directly, view it on GitHub https://github.com/ITensor/ITensorTDVP.jl/issues/27#issuecomment-1275327290, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHJ3RNPCLISMOFDG26VNA3WCXP63ANCNFSM5XSWXGSA . You are receiving this because you commented.Message ID: @.***>

-- -=Miles Stoudenmire=- @. @. http://itensor.org/miles/

[emstoudenmire]

So in working on the linsolve PR I am running into one issue where I can't quite reach the ideal of catching all the kwargs.

The issue is that I run into the following choices:

• I put a precise list of kwargs in my solver definition, since it is the "lowest level of the tower" in the above design suggestion from Ash and I want kwarg misspellings to get caught and throw an error. However, then the generic tdvp function is passing other kwargs not relevant to my solver, which I didn't put and don't want to have to catch, so I get an error.
• To fix this, I add kwargs... to the end of my solver function, but now I've lost the behavior where a misspelled kwarg will be caught.

So maybe this is a fundamental tension with generic code and catching all kwarg misspellings? We could in the solvers just always catch all possible solver arguments, and not slurp (no kwargs...), but then if we add a new kwarg for one solver we'd have to add it to all the solvers and it would make the process of defining new solvers more rigid and difficult.

Do either of know a way around this tensor @amilsted @mtfishman ?

[amilsted]

What are these other kwargs that are being passed to the solver? The idea would be to "pop" them all as we go down the levels, so that only the relevant ones are left at the bottom level. This can be done by just catching the ones we want in the call signature.

There is a caveat I just remembered. All bottom-level functions have to be non-slurping for this to work, so you don't want to pass your kwargs down to a user-supplied callback function.

[emstoudenmire]

First of all, agreed that all of the functions at the bottom of the tower (solvers in this case) would have to be non-slurping. I believe that's manageable at the level of the solvers included with the package. Then if an outside user inputs a solver which is slurping, then they will be taking on the risk that a misspelled kwarg might then get missed, and that's ok because it's just a mechanism of Julia that we don't have a way around.

Good question about the other kwargs that get passed to the solver. A lot do get popped on the way down, or will in an upcoming PR meant to address this issue.

However, here's the issue I'm seeing (both narrowly and broadly):

• right now, when I make the linsolve solver non-slurping, I get an error related to the fact that it isn't taking a kwarg called current_time
• narrowly this could be fixed by either making sure to pop current_time if it isn't meant for solvers
• but if it is meant for solvers (and other kwargs certainly may well be, which are used for some solvers but not by the linsolve one) then we end up in a situation where all the solvers have to take all the solver kwargs. To be clear, linsolve has no use for current_time but in the current situation might have to accept it by name anyway.

So I was just looking for a way around that kind of "rigidity" of the design, where all the solvers have to take all the other solvers' args to work.

Here could be another, and possibly bigger, reason to adopt @mtfishman's point about using NamedTuples. If we make it so that the common kwarg at the bottom, for solvers, is a single kwarg called solver_kwargs which is a NamedTuple then it seems to nicely solve this issue. Because of course there's no problem requiring all the solvers to take that one kwarg and then put whatever sub-kwargs inside it or not as desired.

But none of this was particularly obvious to me before this issue thread, so I am appreciating the back and forth here and I think we are converging on something really good.

[mtfishman]

We should think about two kinds of keyword arguments being passed to solvers:

1. External/user provided keyword arguments, for example ones that get passed from the high level tdvp function (perhaps through the NamedTuple syntax I proposed here: https://github.com/ITensor/ITensorTDVP.jl/pull/37#discussion_r995981402)
2. Internal/context-dependent keyword arguments, for example we may want to pass information about the current time of the evolution or which sweep/step/site we are on, which may be used by some solvers but not others. We should be careful and clear about what information really needs to get passed, but I think there will be cases where this is necessary (for example you need to pass the current time of the evolution to the solver for time-dependent Hamiltonians). You could also imagine a user wanting to change the solver tolerence as a function of sweep/some error measure (this is commonly used in the VUMPS algorithm).

For the "internal keyword arguments", indeed there is the choice that for each solver you either need to list them explicitly or use kwargs... to slurp them, with the disadvantage of losing error messages for misspelled keyword arguments. So for internal solvers, I think we should have the convention that we list all "internal keyword arguments" explicitly. For user defined keyword arguments, they can of course use either approach.

In principle I think we can have a special case of ignoring only "internal keyword arguments" using some Julia introspection tools, we do something like that in Observers.jl (https://github.com/GTorlai/Observers.jl/blob/6f8f1f9e6904d10c14b63618e068781ac4cebad9/src/Observers.jl#L89-L118). Basically you should be able to look up the correct method of the solver, see which keyword arguments that method accepts, and then match that to the list of "internal keyword arguments" and strip the ones it doesn't accept. So we could consider a solution like that.

[mtfishman]

With the NamedTuple interface, I was not proposing passing that to the solvers directly as a NamedTuple, but instead splatting it as the keyword arguments of the solvers, so that wouldn't necessarily do anything to help with this issue.

[emstoudenmire]

Those are helpful clarifications. One that your proposal was more to splat the NamedTuple. That does make sense. What do you think of my suggestion (which might be bad!) of passing the whole NamedTuple so as to get around all the solvers having to take the exact same set of kwargs.

The other clarification is that it does sound like if the solvers don't slurp, that then they all must take exactly the same set of kwargs. It would not be the worst thing in the world, but it does create a sort of "quadratic scaling" issue where to add a new kwarg to one solver you then have to go add it to all the other solver. Plus if a user was using a custom solver such a change would break theirs until they upgraded it (or just made it slurp).

We could live with this, but I'm just clarifying what choice we would be making and making sure we've exhausted all the alternatives.

[mtfishman]

Hmm, I don't think all solvers need to take all the same keyword arguments, only the few "internal keyword arguments" like current_time that can only be passed internally from the library.

[mtfishman]

And as I said, I think there are introspective Julia tools that we could use to check if a solver accepts those specific keyword arguments, and avoid throwing an error (by not passing them to the solver) if they don't.

[mtfishman]

I think passing the entire NamedTuple will make the solver functions messier, since then every function would need to process NamedTuples. It also seems to just push the same issues down one level (i.e. you still need to decide whether to throw errors about keyword arguments that are named incorrectly, whether or not to ignore extra keyword arguments, etc.).

[mtfishman]

This is fixed by https://github.com/ITensor/ITensors.jl/pull/1226.

It's possible that will break some of the code here since now it will throw an error if lower level functions like svd get passed keyword arguments that they aren't supposed to accept. If so it should be easy to fix.