Skip to content

fix passing of curvefit kwargs #6978

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

slevang
Copy link
Contributor

@slevang slevang commented Sep 1, 2022

@TomNicholas
Copy link
Member

Thanks for doing this @slevang ! Would you mind adding a tiny regression test too?

@slevang
Copy link
Contributor Author

slevang commented Sep 2, 2022

Thanks for doing this @slevang ! Would you mind adding a tiny regression test too?

Right, I guess this actually breaks the previous way of passing kwargs and that is why the docs build failed. Hmmm. To go with the current changes, thoughts on adding something like this to the parsing logic:

if kwargs is None:
    kwargs = {}
elif "kwargs" in kwargs:
    kwargs = {**kwargs.pop("kwargs"), **kwargs}

to allow for the desired functionality but also handle the old case when someone passes kwargs as a dict? Feels wonky but it works. Is there a smarter way of doing this?

BTW it took me a minute to figure out what happened here because the docstring in the original PR was actually correct (requiring a dict, albeit maybe not the best way of passing kwargs), but then got changed in #5182 to suggest that kwargs could be passed on their own. I see .interp() behaves the same, where only a dict of kwargs can be provided and passed to the underlying scipy interpolation. So it could be worth generalizing this for other methods where we are passing kwargs to a child function of some sort, to allow both of these patterns.

@headtr1ck
Copy link
Collaborator

headtr1ck commented Sep 3, 2022

Is that something that will be deprecated or is it planned to keep the support for the kwargs dict forever?

@slevang
Copy link
Contributor Author

slevang commented Sep 23, 2022

Is that something that will be deprecated or is it planned to keep the support for the kwargs dict forever?

Not sure if there are any strong opinions here? I don't see much harm in keeping it around but we could also deprecate.

Copy link
Contributor

@Illviljan Illviljan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @slevang !
Deprecation can be added in a later PR once we figure out what we want.

@Illviljan Illviljan added the plan to merge Final call for comments label Sep 27, 2022
@dcherian
Copy link
Contributor

only a dict of kwargs can be provided

I think we may want to keep only the current behaviour (perhaps add an example to the docstring instead), and have that be consistent across the project when we wrap external functions. It's similar to what we do with apply_ufunc also.

Can someone open an issue to discuss this?

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

Successfully merging this pull request may close these issues.

Passing extra keyword arguments to curvefit throws an exception.
5 participants