Skip to content

Conversation

@jokasimr
Copy link
Contributor

@jokasimr jokasimr commented May 28, 2024

Fixes #3463

The inspection code in the curve fit function was complicated and didn't allow partial functions.
This change makes the logic a bit more transparent and allows for partial model functions.

@jokasimr jokasimr requested a review from jl-wynen May 28, 2024 14:34
@jokasimr jokasimr force-pushed the fix-curve-fit-kwonlydefault branch from c605b59 to 2a6dd6e Compare May 28, 2024 14:36
@jokasimr jokasimr changed the title Fix curve fit kwonlydefault Fix curve fit ignore kwonlydefault May 28, 2024
if not set(coords).issubset(args):
raise ValueError("Function must take the provided coords as arguments")
default_arguments = dict(
zip(spec.args[-len(spec.defaults) :], spec.defaults, strict=True)
Copy link
Member

Choose a reason for hiding this comment

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

What about spec.defaults?

Copy link
Contributor Author

@jokasimr jokasimr May 29, 2024

Choose a reason for hiding this comment

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

The idea is that we are doing (almost) the same as scipy curve fit here, that is, ignoring default values unless they are kwargonly.

Example:

def model(x, a, c=3, *, d, e=5):
    return ...

Let's say x is a coordinate. Then a, c and d will be counted as parameters here in curve_fit.
e will not be counted as a parameter because it is kwargonly and has a default value, and might have been set by partial.
The default value of c will be discarded because it is not a kwargonly argument.

We differ from scipy because we also allow kwargonly arguments without defaults to be counted as parameters. That is, scipy would only have counted a and c as parameters in the above example.

Copy link
Member

Choose a reason for hiding this comment

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

The default value of c will be discarded because it is not a kwargonly argument.

Yes. My question is, why do we do that?

Also see #3465 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that is what scipy does here, and I think it makes sense to try to do the same thing there.

Copy link
Member

Choose a reason for hiding this comment

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

Scipy does not allow keyword-only arguments. And it never seems to respect default arguments. So this argument does not work. We are deviating from Scipy by allowing keyword-only arguments. In the Scipy wrapper, we even require keyword-only arguments.

So my questions are:

  • Why would scipp.curve_fit allow positional arguments?
  • Why would scipp.curve_fit tread positional and keyword-only arguments differently if both are allowed?

Copy link
Contributor Author

@jokasimr jokasimr May 29, 2024

Choose a reason for hiding this comment

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

Scipy does not allow keyword-only arguments. And it never seems to respect default arguments.

But it does? Scipy respects kwargonlydefault arguments, but not default arguments.
See here:

from scipy.optimize import curve_fit
curve_fit(lambda x, a, *, b=5: x * a, [0, 1, 2], [4, 5, 6])
# returns 1 parameter value, not 2

It's true that Scipy does not allow keyword-only arguments. Scipy curve_fit will fail on functions with kwargonly arguments without default values.
Scipp doing something different in that case is not a deviation from scipy, more an extension, which I think makes sense in this case.

Answers to the questions:

Why would scipp.curve_fit allow positional arguments?

Why would it not? There's no reason to disallow it, and creating artificial constraints harms usability.

Why would scipp.curve_fit tread positional and keyword-only arguments differently if both are allowed?

Yes I see that this is a bit strange. But I think all the choices here have drawbacks. If we want to always treat positional and keyword arguments the same way we could either:

  • respect default (non-keyword) arguments - but this differs significantly from scipy
  • only allow parameters to be keyword arguments - makes the interface more cumbersome

spec = getfullargspec(f)
all_args = {*spec.args, *spec.kwonlyargs}
if not set(coords).issubset(all_args):
args = (set(spec.args) | set(spec.kwonlyargs)) - set(spec.kwonlydefaults or ())
Copy link
Member

Choose a reason for hiding this comment

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

scipp.scipy.optimize.curve_fit requires keyword-only args for all but the first argument. Is scipp.curve_fit different? If not, spec.args can be removed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes scipp.curve_fit is different in that regard.

Copy link
Member

Choose a reason for hiding this comment

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

Please document this behaviour clearly! It is not clear from the signature how model arguments are handled.

Copy link
Member

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

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

A small change but then it's ok.

Comment on lines 137 to 141
non_default_args = (
spec.args[: -len(spec.defaults)] if spec.defaults is not None else spec.args
)
args = (
{*non_default_args, *spec.kwonlyargs}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
non_default_args = (
spec.args[: -len(spec.defaults)] if spec.defaults is not None else spec.args
)
args = (
{*non_default_args, *spec.kwonlyargs}
args = (
{*spec.args, *spec.kwonlyargs}

given that the defaults are removed below.

@jokasimr jokasimr force-pushed the fix-curve-fit-kwonlydefault branch from de3928e to d5a68d8 Compare May 30, 2024 07:25
@jokasimr jokasimr force-pushed the fix-curve-fit-kwonlydefault branch from d5a68d8 to 1cc552f Compare May 30, 2024 14:19
@jokasimr jokasimr force-pushed the fix-curve-fit-kwonlydefault branch from 1cc552f to 2e0dc63 Compare May 30, 2024 14:19
@jokasimr jokasimr merged commit 0525e3b into main May 30, 2024
@jokasimr jokasimr deleted the fix-curve-fit-kwonlydefault branch May 30, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How to handle default arguments in fit function in curve_fit?

3 participants