Add recharge to gdp callback

[DavidLitwin]

I added recharge rate as argument to the callback function. This is probably the simplest solution to obtaining the recharge rate for each substep when it cannot be guaranteed to exist as a grid field. While recharge rate doesn't change during substeps, I've found it difficult to reconstruct the subtimestep recharge rates when a long series of stochastic recharge events are run.

[coveralls]

Coverage increased (+0.5%) to 85.144% when pulling 9b1e073 on dlitwin/gdp_callback into 447be81 on master.

[coveralls]

Coverage decreased (-0.04%) to 84.599% when pulling 244eafe on dlitwin/gdp_callback into 8a85b7a on master.

[kbarnhart]

This is on my todo list! Probably will get to it tomorrow @DavidLitwin.

[kbarnhart]

For backward compatibility, I think that this needs to take either 2 or 3 arguments.

1. The docs should state this, and that the two option callback function is deprecated.

2. In the code itself the number of arguments should get tested. One example of testing the number of arguments a function takes is given at line 168 of landlab\components\lithology\litholayers.py @mcflugen may know a better way to do this.

3. If two arguments are provided then a DeprecationWarning should be raised and grid, dt passed. If three, then grid, recharge, dt passed. (Sidenote: Raising the DeprecationWarning will be helpful for whoever manages the next major version because it will help them find that this is a change that was deprecated).

4. Add one test, so you have one test with a three element function and one with a two element function (that yields a deprecation warning). At some point I remember it being tricky to figure out how to test for deprecated warnings. Here is a link to the part of pytest that shows an example.

[DavidLitwin]

Thanks for taking a look Katy. I'm trying out the method function.__code__.co_varnames, and the one problem I see is that it doesn't distinguish between arguments and keyword arguments. There can be as many keyword arguments as the user wants, e.g. to supply the name of the output file. I'm looking into using the signature class from inspect, but I'm still not quite sure how to separate the two. For example, someone could provide a function something like:

def callback(grid, recharge, dt, d='file.csv'):
    return np.save_txt(d, grid['surface_water_specific__discharge']*dt)

And that would be fine.

[kbarnhart]

Will think on this @DavidLitwin... though @mcflugen is the best at this sort of thing.

[DavidLitwin]

@kbarnhart, @mcflugen had some good comments over here, which I've started to implement. Is this the kind of functionality you had in mind? I will try to add a test later today.

[kbarnhart]

@DavidLitwin it felt easier to make changes/comment by editing the file directly. Feel free to roll back any of my changes if you don't like them. Also, some of the changes are just comments that I expect you to delete.

A function may have keyword arguments set in its definition, but the way that the callback function is actually used, there is no mechanism to pass keyword arguments to the function (so defaults are always used, yes). So I think its as easy as testing whether it works with three args, or two args, and then if neither works, raising an actual error. So I think what you have done is 95% there! 👍

LMK if you have any questions about what I've written.

[DavidLitwin]

Hi @kbarnhart, this looks good, thank you! I guess I'm a little unsure on the use of keyword arguments. The way I've been using this thing, I only ever use the defaults of the keyword arguments. But if I add callback_fun(grid, recharge_rate, dt, **kwargs), what does that allow me/a user to do?

I trust your judgement on the ValueError call, I don't know very much about errors yet. One thing that might be unclear currently is that the variable error in line 415 will return callback_fun takes 2 positional arguments but XX were given, which is different from the statement that follows that says to provide the three arguments...

[kbarnhart]

note: I wrote this really fast because I wanted to get you an answer today instead of in two days. so if things don't make sense, assume that is why.

re kwds:

lets say you define a function with a signature

def myfun(spam, eggs=3):

And you pass that as a function to be used within another function.

def big_fun(a, b, c, callback_fun=...):
    spam = 1
    callback_fun(spam)

Unless the input to `big_function has an input for a dictionary to pass as keyword arguments to the internally called callback function, your function is equivalent to:

def myfun(spam):
    eggs = 3 # put all defaults here 

Providing the ability to pass keyword arugments might look like:

def big_fun(a, b, c, callback_fun=..., **kwds):
    spam = 1
    callback_kwds = {}
    if "eggs" in kwds:
        callback_kwds["eggs"] = kwds.pop("eggs")
    callback_fun(spam)
   at end of function raise an unused keyword error if there is stuff still left in kwds. 

(you'd do it this way if there might be other functions that you pass keywords too)

Or, if the callback function is the only thing you might need to pass keywords to:

def big_fun(a, b, c, callback_fun=..., **kwds):
    spam = 1
    callback_fun(spam, **kwds)

Importantly, in both of these examples, there is some source of **kwds... As best as I can tell, in your application there is no mechanism by which kwds are passed into the run_one_step or init such that they can be passed to the callback function.

re: value error

My thinking was, if a callback function couldn't deal with being passed 2 or 3 arguments, then it would break upon use. That is a thing a user should find out as soon as GDP attempts to set the callback function, not upon running.

[DavidLitwin]

@kbarnhart sorry for the delayed reply. Thank you for the lesson on keyword arguments! I've included a way to set them, and added a statement in the final test that makes sure that they are taken.

[kbarnhart]

@DavidLitwin this looks great. One minor comment which is to make callback_kwds private.

I also have a question fro @mcflugen that I think won't require any changes, but if it does, they'd be minor.

@mcflugen - my question is if there should be anything else done here to verify that no unused keyword arguments are passed. We often use pop, and then check that **kwds is empty at the end of the init... However, in this case, @DavidLitwin saves **callback_kwds and passes them to the callback_fun in its setter (I think this is the right approach). And if there were unused **kwds, then this should raise an error... So I think its OK.

[DavidLitwin]

@kbarnhart great, I think I've addressed everything now, save a comment from @mcflugen.

[kbarnhart]

@DavidLitwin I think this looks good. There was a transient link-check error that caused the docs stage to fail, but other than that all the tests pass!

Since I think it would be good for @mcflugen to take a quick look I've added him as a reviewer.

[DavidLitwin]

@mcflugen when you have a chance could you take a look at this?

[DavidLitwin]

@kbarnhart what do you think about going ahead with this?

[kbarnhart]

done.