ContextLab / hypertools

A Python toolbox for gaining geometric insights into high-dimensional data
http://hypertools.readthedocs.io/en/latest/
MIT License
1.83k stars 160 forks source link

handle keyword arguments in a more organized way #102

Closed andrewheusser closed 7 years ago

andrewheusser commented 7 years ago

this is currently a bit of a mess. it would be nice to refactor the way we handle keyword arguments

jeremymanning commented 7 years ago

@andrewheusser: i did some code review this morning. currently, in each function we have a series of if statements that look for (and remove) specified keywords from the kwargs dictionary as it gets passed around.

a more elegant solution might be to specify a new function-specific dictionary of default values as follows at the beginning of each function:

argdict['keyword_name'] = default_val

you'll also need myargs = argdict.keys()

Then if you call argdict.update(kwargs) it'll add any new keys in kwargs that weren't in argdict, overwriting any default values in argdict with whatever's in kwargs. You could then loop over myargs, setting each argument to the value in argdict using eval statements:

for x in myargs: eval(x + '=' str(argdict[x]))

note: idea inspired by this stack exchange post.

andrewheusser commented 7 years ago

ah that sounds like a great idea! we coul even separate out argsdict to a separate file, so a use can easily create their own 'defaults' without messing with the plot function, and provide a few alternative default settings. (i think we discussed this or something like it on the slack channel)

andrewheusser commented 7 years ago

this would be wrapped in a function like load_defaults() or something

jeremymanning commented 7 years ago

a defaults idea could be nice-- that way we could define a single function and call args = load_defaults(kwargs) at the beginning of every function-- where whatever's in kwargs overwrites the defaults, but otherwise the defaults are used.

jeremymanning commented 7 years ago

(i don't think we necessarily need to give the user access to it-- e.g. if they install via pip i don't think there's a convenient place to modify or view the code)

jeremymanning commented 7 years ago

or even better, load_defaults(kwargs) would attach all of those variables to the local workspace via eval statements

andrewheusser commented 7 years ago

one more things to consider:

The reason to stick with the *args **kwargs pattern is that whatever is not processed by hypertools is eventually passed on to matplotlib, so users can essentially use our API + all the options offered by hypertools. Thus, hypertools-specific kwargs and matplotlib-specific args and kwargs have be be sorted out internally, or else matplotlib will throw an error when calling ax.plot(data, *args, **kwargs). This is why I was assigning local variables, and then deleting them from the kwargs dict.

jeremymanning commented 7 years ago

we could still remove any keys in myargs from kwargs before passing that dictionary on to matplotlib...right?

andrewheusser commented 7 years ago

yea, we could. i guess that still just feels hacky to me, but itll work

andrewheusser commented 7 years ago

id prefer that our kwargs are handled explicitly, and all matplotlib kwargs are handled with a **kwargs at the end of the function def, like seaborn does:

def tsplot(data, time=None, unit=None, condition=None, value=None,
           err_style="ci_band", ci=68, interpolate=True, color=None,
           estimator=np.mean, n_boot=5000, err_palette=None, err_kws=None,
           legend=True, ax=None, **kwargs):
    """Plot one or more timeseries with flexible representation of uncertainty.

all the explicit keywords are seaborn specific, and the rest are matplotlib, but this unfortunately doesn't work because we want to pass in unlabeled(?) args like 'o'. If we switched it to hyp.plot(data, marker='o') that would fix the problem, but also would be a major chance to the API

andrewheusser commented 7 years ago

ahhhh, this issue is solved for python 3 but not 2!

http://stackoverflow.com/questions/4372346/uses-of-combining-kwargs-and-key-word-arguments-in-a-method-signature

andrewheusser commented 7 years ago

OK so maybe the best thing to do is to handle kwargs like @jeremymanning suggested above for now, and then when (if?) we eventually deprecate support for python 2, we can clean up the way things are handled

jeremymanning commented 7 years ago

Sounds good!

jeremymanning commented 7 years ago

note: i've started to work on this issue in this branch, but the code is currently non-functional; happy for someone else to take over if interested and able. in the mean time i'll make gradual progress...