Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

filter_kwargs parameter name collision #165

Closed
bmcfee opened this issue Feb 4, 2016 · 6 comments
Closed

filter_kwargs parameter name collision #165

bmcfee opened this issue Feb 4, 2016 · 6 comments
Labels

Comments

@bmcfee
Copy link
Collaborator

bmcfee commented Feb 4, 2016

I'm trying to use util.filter_kwargs on sonify functions, and hitting a collision with the argument function.

This would be fixed by renaming filter_kwargs's function argument (not a keyword argument) to something unique, like wrapped_function.

This should be a trivial change, and I can PR if it sounds reasonable.

@bmcfee bmcfee added the bug label Feb 4, 2016
@bmcfee
Copy link
Collaborator Author

bmcfee commented Feb 4, 2016

.. sorry, just realized that report wasn't terribly precise. The issue is that you can't use filter_kwargs on parameters whose names coincide with those of filter_kwargs itself, since you would have to have multiple values for the colliding names.

@craffel
Copy link
Owner

craffel commented Feb 4, 2016

I'm trying to use util.filter_kwargs on sonify functions

I'm curious what the context is.

This would be fixed by renaming filter_kwargs's function argument (not a keyword argument) to something unique, like wrapped_function.

I see. That's no problem, of course. Though maybe we should be extra sure that a collision with the arg name doesn't happen, and call it something like wrapped_function_87h12ry3b7926fyubun38796tyiuh2ig5uwte87v6fyvub2in5 (/s but only sort of).

@bmcfee
Copy link
Collaborator Author

bmcfee commented Feb 4, 2016

I'm curious what the context is.

time_frequency uses function as the name of its synthesis generator.

Though maybe we should be extra sure that a collision with the arg name doesn't happen, and call it something like

The real solution here is to not use **, and keep the target kwargs as a dict. But that breaks backward-compatibility.

Otherwise, I'd recommend something that connotes a private name, eg, __function. But it's your call.

@craffel
Copy link
Owner

craffel commented Feb 4, 2016

time_frequency uses function as the name of its synthesis generator.

Of course, what I meant was I am curious why you are using filter_kwargs on time_frequency.

Otherwise, I'd recommend something that connotes a private name, eg, __function. But it's your call.

I like your one-underscore solution.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Feb 4, 2016

Of course, what I meant was I am curious why you are using filter_kwargs on time_frequency.

Oh! The upcoming jams.sonify module does dynamic dispatch to map annotation objects to mir_eval.sonify functions. Since we don't (can't) know the sonification type up front, all sonify parameters live in a kwargs, which needs to be filtered because the sonify functions don't have a common signature.

For instance, I have this fragment for making a full-track sonification of all isophonics annotations in one go:

y_annotated = y.copy()
for ann in jam.annotations:
    try:
        print('Trying to sonify {:s}'.format(ann.namespace))
        y_ann = jams.sonify.sonify(ann)
        y_ann = librosa.util.fix_length(y_ann, len(y_annotated))
        y_annotated += y_ann
    except jams.NamespaceError:
        pass

And I'd like to be able to say jams.sonify.sonify(ann, function=scipy.signal.square) without it barfing when it hits a call to click instead of chords (or what-have-you).

@craffel
Copy link
Owner

craffel commented Feb 4, 2016

Oh, nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants