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

Form of callback in new Python binding #71

Open
ivan-pi opened this issue May 22, 2022 · 6 comments
Open

Form of callback in new Python binding #71

ivan-pi opened this issue May 22, 2022 · 6 comments
Labels
bindings: Python API Bindings for Python

Comments

@ivan-pi
Copy link
Member

ivan-pi commented May 22, 2022

In the newly added Python bindings, the expected callback is of form fcn(x, fvec) -> None where fvec is an output vector, of the same size as x, which is modified in-place. The full set of Python callback interfaces is defined here: python/minpack/typing.py

The SciPy least-squares on the other hand, expects a callback with the following description:

Function which computes the vector of residuals, with the signature fun(x, *args, **kwargs), i.e., the minimization proceeds with respect to its first argument. The argument x passed to this function is an ndarray of shape (n,) (never a scalar, even for n=1). It must allocate and return a 1-D array_like of shape (m,) or a scalar. If the argument x is complex or the function fun returns complex residuals, it must be wrapped in a real function of real arguments, as shown at the end of the Examples section.

Is the reason for the different interfaces since the present Fortran MINPACK interface cannot handle extra arguments?

Is the idea that the SciPy layer on top of the new Python binding, would nest a small adaptor function?

    def _callback_hy(x,fev) -> None:
        fev[:] = fun(x, *args, **kwargs)
@awvwgk
Copy link
Member

awvwgk commented May 22, 2022

It's a matter of design, I decided to stay close to the Fortran callback, because it does not require additional allocations of ndarrays in the callback.

There is no problem with creating a callback using fun(x, *args, **kwargs) and pass through the extra arguments. The issue / annoyance is that it is more difficult to correctly identify the dimensions ahead of time. In SciPy I think the function has to be called once before the Minpack driver can be invoked (could be the cause for issues like scipy/scipy#5369).

@awvwgk awvwgk added the bindings: Python API Bindings for Python label May 22, 2022
@awvwgk
Copy link
Member

awvwgk commented May 22, 2022

We can of course add the possibility to pass-through *args and **kwargs if wanted, this shouldn't be an issue at all.

@ivan-pi
Copy link
Member Author

ivan-pi commented May 22, 2022

I forgot to add a question label. I didn't want to imply we should (or should not) follow the SciPy conventions. It was more a matter of where/how do we draw a line between our bindings, and the established SciPy interface in a way that will ease maintenance and minimize the number of interface crossings. For example do we reject handling *args and **kwargs as a matter of "policy"? We do so currently in the Fortran and C bindings.

If I understand correctly, there are entire files in the SciPy MINPACK interface, such as https://github.com/scipy/scipy/blob/main/scipy/optimize/__minpack.h and https://github.com/scipy/scipy/blob/main/scipy/optimize/_minpackmodule.c, which would become obsolete if the bindings in this repository were to be used instead in the future.

@awvwgk
Copy link
Member

awvwgk commented May 22, 2022

Adding **kwargs support is easy (see #73), the transformative logic applied to the callbacks and drivers requires changes at only a few places. Sadly, updating the typing information for the callbacks is the most involved and complicated step for the **kwargs support.

@ivan-pi
Copy link
Member Author

ivan-pi commented May 22, 2022

That's an argument against handling **kwargs, since it can be handled in the upstream SciPy interface with a little glue code.

I guess it depends on what is the role of the Python bindings. If it's just for SciPy, then I would let them handle it. But if we want the bindings also for other Python packages (who may want to stick with smaller and more specific dependencies than SciPy), then **kwargs would be nice to have.

If we handle **kwargs but not *args, SciPy will still need an adaptor. 🤷🏻

@certik
Copy link
Member

certik commented May 22, 2022

I think we want to have the Python bindings working for everyone. But SciPy is an important "customer", so we want to make sure they work great there as well.

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

No branches or pull requests

3 participants