BSplineX-python new wrapper

[SebastianoTaddei]

Let's see if this works as I'd like it to. I hope to be able to link commits and such. Let's use this to discuss the best way to write the new wrapper.

See branch features/wrapper for a taste on the new wrapper. In short I propose the following:

from . import _bsplinex_impl as _impl

from typing import overload
from abc import ABC
from textwrap import dedent

import numpy as np
import numpy.typing as npt


class BSpline(ABC):
    def __init__(self, bspline: _impl.OpenUniform | _impl.OpenNonUniform | None) -> None:
        super().__init__()
        if bspline is not None:
            self._bspline = bspline

    @overload
    def evaluate(self, x: float, derivative_order: int = 0) -> float: ...
    @overload
    def evaluate(self, x: npt.NDArray[np.float64], derivative_order: int = 0) -> npt.NDArray[np.float64]: ...

    def evaluate(
        self, x: npt.NDArray[np.float64] | float, derivative_order: int = 0
    ) -> npt.NDArray[np.float64] | float:
        """evaluate the b-spline (derivative) at the given value(s)

        :param x: value(s) to evaluate the b-spline at
        :param derivative_order: order of the derivative, default 0
        :return: the value(s) of the b-spline
        """

        return self._bspline.evaluate(x, derivative_order)


class OpenUniform(BSpline):
    pass


class OpenNonUniform(BSpline):
    pass


@overload
def make_open_uniform(degree: int) -> OpenUniform: ...


@overload
def make_open_uniform(degree: int, begin: float, end: float, num_elems: int) -> OpenUniform: ...


@overload
def make_open_uniform(
    degree: int, begin: float, end: float, num_elems: int, ctrl_points: npt.NDArray[np.float64] | list[float]
) -> OpenUniform: ...


def make_open_uniform(
    degree: int,
    begin: float | None = None,
    end: float | None = None,
    num_elems: int | None = None,
    ctrl_points: npt.NDArray[np.float64] | list[float] | None = None,
) -> OpenUniform:
    """Create a uniform b-spline with open boundary condition

    This function has three possible overloads:

    - make_open_uniform(degree)
    - make_open_uniform(degree, begin, end, num_elems)
    - make_open_uniform(degree, begin, end, num_elems, ctrl_points)

    Note that only `make_open_uniform(degree, begin, end, num_elems, ctrl_points)`
    generates a valid b-spline, the other overloads are meant to
    be used for fitting (`make_open_uniform(degree, begin, end, num_elems)`)
    and interpolation (`make_open_uniform(degree))`

    :param degree: degree of the b-spline
    :param begin: starting knot
    :param end: end knot
    :param num_elems: number of knots
    :param ctrl_points: control points
    :return: the open uniform b-spline
    """

    if all((arg is not None for arg in (begin, end, num_elems, ctrl_points))):
        return OpenUniform(_impl.make_open_uniform(degree, begin, end, num_elems, ctrl_points))
    if all((arg is not None for arg in (begin, end, num_elems))) and ctrl_points is None:
        return OpenUniform(_impl.make_open_uniform(degree, begin, end, num_elems))
    if all((arg is None for arg in (begin, end, num_elems, ctrl_points))):
        return OpenUniform(_impl.make_open_uniform(degree))

    error = """
    This function has three possible overloads:
    - make_open_uniform(degree)
    - make_open_uniform(degree, begin, end, num_elems)
    - make_open_uniform(degree, begin, end, num_elems, ctrl_points)
    """

    raise ValueError(dedent(error))


@overload
def make_open_nonuniform(degree: int) -> OpenNonUniform: ...


@overload
def make_open_nonuniform(
    degree: int,
    knots: npt.NDArray[np.float64] | list[float] | None = None,
) -> OpenNonUniform: ...


@overload
def make_open_nonuniform(
    degree: int,
    knots: npt.NDArray[np.float64] | list[float] | None = None,
    ctrl_points: npt.NDArray[np.float64] | list[float] | None = None,
) -> OpenNonUniform: ...


def make_open_nonuniform(
    degree: int,
    knots: npt.NDArray[np.float64] | list[float] | None = None,
    ctrl_points: npt.NDArray[np.float64] | list[float] | None = None,
) -> OpenNonUniform:
    """Create a nonuniform b-spline with open boundary condition

    This function has three possible overloads:

    - make_open_nonuniform(degree)
    - make_open_nonuniform(degree, knots)
    - make_open_nonuniform(degree, knots, ctrl_points)

    Note that only `make_open_nonuniform(degree, knots, ctrl_points)`
    generates a valid b-spline, the other overloads are meant to
    be used for fitting (`make_open_uniform(degree, knots)`)
    and interpolation (`make_open_uniform(degree))`

    :param degree: degree of the b-spline
    :param knots: knots
    :param ctrl_points: control points
    :return: the open nonuniform b-spline
    """

    if all((arg is not None for arg in (knots, ctrl_points))):
        return OpenNonUniform(_impl.make_open_nonuniform(degree, knots, ctrl_points))
    if knots is not None and ctrl_points is None:
        return OpenNonUniform(_impl.make_open_nonuniform(degree, knots))
    if all((arg is None for arg in (knots, ctrl_points))):
        return OpenNonUniform(_impl.make_open_nonuniform(degree))

    error = """
    This function has three possible overloads:
    - make_open_nonuniform(degree)
    - make_open_nonuniform(degree, knots)
    - make_open_nonuniform(degree, knots, ctrl_points)
    """

    raise ValueError(dedent(error))

[masinag]

Overall, I think the wrapper is a good idea, and the draft shows how necessary it is.

One thing I don't like are dummy subclasses

class OpenUniform(BSpline):
    pass
class OpenNonUniform(BSpline):
    pass

Since they have no actual meaning besides (maybe) helping readability.

I had some alternative solutions in mind, for instance, we could get completely rid of the factory functions in the Python interface, and overload the constructors of the subclasses instead.
However, this would make the Python interface quite different from the C++ interface, which I am not sure is something we want.
I'll think about it tonight 💤

[SebastianoTaddei]

Yeah they are dummy for now, but I would like to provide functionality as well. The idea to drop the factory functions would probably be more Pythonic, let's think about it.

[masinag]

What kind of functionalities were you thinking of for the subclasses?

[SebastianoTaddei]

I still don't know. Maybe better error handling or some kind of helper functions. Although better errors should first be implemented in C++ (just opened a new issue). I'll think about it today and tomorrow.

[SebastianoTaddei]

For example, we could have these constructors:

class OpenUniform(BSpline):
    @overload
    def __init__(self, degree: int) -> None:
        """Create a uniform b-spline with open boundary condition

        > NOTE: useful constructor for interpolating points

        :param degree: degree of the b-spline
        :return: the open uniform b-spline
        """
        ...

    @overload
    def __init__(self, degree: int, begin: float, end: float, num_elems: int) -> None:
        """Create a uniform b-spline with open boundary condition

        > NOTE: useful constructor for fitting points

        :param degree: degree of the b-spline
        :param begin: starting knot
        :param end: end knot
        :param num_elems: number of knots
        :return: the open uniform b-spline
        """
        ...

    @overload
    def __init__(
        self, degree: int, begin: float, end: float, num_elems: int, ctrl_points: npt.NDArray[np.float64] | list[float]
    ) -> None:
        """Create a uniform b-spline with open boundary condition

        :param degree: degree of the b-spline
        :param begin: starting knot
        :param end: end knot
        :param num_elems: number of knots
        :param ctrl_points: control points
        :return: the open uniform b-spline
        """
        ...

    def __init__(
        self,
        degree: int,
        begin: float | None = None,
        end: float | None = None,
        num_elems: int | None = None,
        ctrl_points: npt.NDArray[np.float64] | list[float] | None = None,
    ) -> None:
        """Create a uniform b-spline with open boundary condition

        This function has three possible overloads:

        - make_open_uniform(degree)
        - make_open_uniform(degree, begin, end, num_elems)
        - make_open_uniform(degree, begin, end, num_elems, ctrl_points)

        Note that only `make_open_uniform(degree, begin, end, num_elems, ctrl_points)`
        generates a valid b-spline, the other overloads are meant to
        be used for fitting (`make_open_uniform(degree, begin, end, num_elems)`)
        and interpolation (`make_open_uniform(degree))`

        :param degree: degree of the b-spline
        :param begin: starting knot
        :param end: end knot
        :param num_elems: number of knots
        :param ctrl_points: control points
        :return: the open uniform b-spline
        """

        if all((arg is not None for arg in (begin, end, num_elems, ctrl_points))):
            super().__init__(_impl.make_open_uniform(degree, begin, end, num_elems, ctrl_points))
            return
        if all((arg is not None for arg in (begin, end, num_elems))) and ctrl_points is None:
            super().__init__(_impl.make_open_uniform(degree, begin, end, num_elems))
            return
        if all((arg is None for arg in (begin, end, num_elems, ctrl_points))):
            super().__init__(_impl.make_open_uniform(degree))
            return

        error = """
        This function has three possible overloads:
        - make_open_uniform(degree)
        - make_open_uniform(degree, begin, end, num_elems)
        - make_open_uniform(degree, begin, end, num_elems, ctrl_points)
        """

        raise ValueError(dedent(error))

I tested them and the overloads working (now also the docstring per overload). We need to understand if it actually works because it has happened to me before that it randomly stopped showing the docstrings correctly for the overloads. that being said nvim does a good job at showing them and vscode an exceptional one, feels like c++ overloads.

Obviously this is to be ironed out, I do not like the triple if in the constructor, but the idea is there I think.

I still have no idea what kind of functionality we may add, but this direction feels way more pythonic, I think I like it.
@masinag thoughts?

Anyway, I am now settled and ready to work on this again 😉 🚀

[SebastianoTaddei]

Ah, the docstring of the actual implementation is shadowed, so maybe we can do without (or maybe just write something useful internally like the legal combinations of parameters or something

[masinag]

I think it's not bad, still it does not feel like the ultimate super-elegant solution but it could be a good way to start for now.
Probably it is also because the C++ interface could be improved or completely rethought.
Maybe in the making we will figure out something better.

Ah, the docstring of the actual implementation is shadowed, so maybe we can do without (or maybe just write something useful internally like the legal combinations of parameters or something.

I would remove it, I don't think it makes sense to have two docstrings for the same method.

Anyway, I am now settled and ready to work on this again 😉 🚀

🚀

Btw, I was thinking that if we don't need extra functionalities, an alternative is to have only one class BSpline, keeping the factory functions, and possibly rethinking them:

• rename factory functions from make_open_uniform to OpenUniform to resemble the constructor of a class (with less overhead).
• switch from factory functions to factory methods, to be used like BSpline.make_open_uniform(...)

Implementation-wise, they are not very different, we could switch from one to the other if we change our mind.

[masinag]

So, I think we can start implementing what you sketched above (remember to replace make_open_uniform with __init__ also in the docstrings).

[SebastianoTaddei]

The thing is I think any extra functionality would be the same for all of them. So we could try having just the BSpline class, I thought about it too. This way it sort of feels like having methods inside namespaces (since I think you meant for them the be "classmethod"). I'll sketch something either tomorrow or once I get internet at our place.

[SebastianoTaddei]

So maybe we could do one more sketch, see how it feels, and then decide on which way to go.

[SebastianoTaddei]

How about something like this

class BSpline:
    _bspline: _impl.OpenUniform | _impl.OpenNonUniform

    def __init__(self, bspline: _impl.OpenUniform | _impl.OpenNonUniform) -> None:
        self._bspline = bspline

    @overload
    def evaluate(self, x: float, derivative_order: int = 0) -> float: ...
    @overload
    def evaluate(self, x: npt.NDArray[np.float64], derivative_order: int = 0) -> npt.NDArray[np.float64]: ...

    def evaluate(
        self, x: npt.NDArray[np.float64] | float, derivative_order: int = 0
    ) -> npt.NDArray[np.float64] | float:
        """evaluate the b-spline (derivative) at the given value(s)

        :param x: value(s) to evaluate the b-spline at
        :param derivative_order: order of the derivative, default 0
        :return: the value(s) of the b-spline
        """

        return self._bspline.evaluate(x, derivative_order)

    @overload
    @classmethod
    def make_open_uniform(cls, degree: int) -> "BSpline":
        """Create a uniform b-spline with open boundary condition

        > NOTE: useful constructor for interpolating points

        :param degree: degree of the b-spline
        :return: the open uniform b-spline
        """
        ...

    @overload
    @classmethod
    def make_open_uniform(cls, degree: int, begin: float, end: float, num_elems: int) -> "BSpline":
        """Create a uniform b-spline with open boundary condition

        > NOTE: useful constructor for fitting points

        :param degree: degree of the b-spline
        :param begin: starting knot
        :param end: end knot
        :param num_elems: number of knots
        :return: the open uniform b-spline
        """
        ...

    @overload
    @classmethod
    def make_open_uniform(
        cls, degree: int, begin: float, end: float, num_elems: int, ctrl_points: npt.NDArray[np.float64] | list[float]
    ) -> "BSpline":
        """Create a uniform b-spline with open boundary condition

        :param degree: degree of the b-spline
        :param begin: starting knot
        :param end: end knot
        :param num_elems: number of knots
        :param ctrl_points: control points
        :return: the open uniform b-spline
        """
        ...

    @classmethod
    def make_open_uniform(
        cls,
        degree: int,
        begin: float | None = None,
        end: float | None = None,
        num_elems: int | None = None,
        ctrl_points: npt.NDArray[np.float64] | list[float] | None = None,
    ) -> "BSpline":

        if all((arg is not None for arg in (begin, end, num_elems, ctrl_points))):
            return cls(_impl.make_open_uniform(degree, begin, end, num_elems, ctrl_points))
        if all((arg is not None for arg in (begin, end, num_elems))) and ctrl_points is None:
            return cls(_impl.make_open_uniform(degree, begin, end, num_elems))
        if all((arg is None for arg in (begin, end, num_elems, ctrl_points))):
            return cls(_impl.make_open_uniform(degree))

        error = """
        This function has three possible overloads:
        - make_open_uniform(degree)
        - make_open_uniform(degree, begin, end, num_elems)
        - make_open_uniform(degree, begin, end, num_elems, ctrl_points)
        """

        raise ValueError(dedent(error))

[masinag]

Looks pretty good to me. I have some questions/comments:

• I am not sure if making them factory methods is a good idea. Maybe we should keep the interface more similar to the C++ one. What do you think? I know I proposed this, and I am not totally against it, I was just rethinking about it 😄
• I see that you used a single docstring for BSpline.evaluate, and multiple docstrings for the overloads of BSpline.make_open_uniform.
  • How are the multiple docstrings shown in the docs? Do you have to choose which overload you want the docs of?
  • Should we stay consistent or is it ok to use single/multiple depending on the specific scenario?
• If we keep it this way (and maybe anyway), we have to find a way to make the constructor private

[SebastianoTaddei]

So you are suggesting to go back to the initial mockup?

I believe that if the docstring is missing it should pick up the latest one, but I need to check. If it does I believe it makes sense for methods which are not that different from each other, but maybe even for evaluate it makes sense to split them. What do you think?

BTW I finally have internet at my new place, I can finally work on it on the weekend/evening

[masinag]

So you are suggesting to go back to the initial mockup?

Basically yes, but without the dummy subclasses.

If we don't have very good reasons to have two different interfaces, then I would use the same.
And passing from factory functions to methods doesn't seem to bring any particular improvement.
Moreover, this seems similar to what numpy does with np.zeros, np.ones etc.

Alternatively, also the interface with the subclasses with different __init__ methods is not bad.
I would go for one of these two.

What do you think?

I believe that if the docstring is missing it should pick up the latest one, but I need to check. If it does I believe it makes sense for methods which are not that different from each other, but maybe even for evaluate it makes sense to split them. What do you think?

If it is clear that the function/method is overloaded, and which overloads are available, then yes, we can split them.
To check also how to show them in the docs (in the website).

[SebastianoTaddei]

Ok let's see if I got this right. If you are proposing to have:

• One BSpline class that encapsulates all types (open, clamped, etc)
• All the factory methods build the correct type but output the BSpline class obviously

If that's the case then I agree. This sort of mimics what we have in C++ (one templated bspline after all)

[masinag]

Yes, correct

[SebastianoTaddei]

Here is hopefully the last draft, let me know what you think @masinag

class BSpline:
    _bspline: _impl.OpenUniform | _impl.OpenNonUniform

    def __init__(self, bspline: _impl.OpenUniform | _impl.OpenNonUniform) -> None:
        self._bspline = bspline

    def evaluate(self, x: npt.ArrayLike, derivative_order: int = 0) -> npt.NDArray[np.float64]:
        """evaluate the b-spline (derivative) at the given value(s)

        :param x: values to evaluate the b-spline at
        :param derivative_order: order of the derivative
        :return: the values of the b-spline evalutated for each x
        """

        return np.asarray(self._bspline.evaluate(x, derivative_order))


@overload
def make_open_uniform(degree: int) -> BSpline:
    """Create a uniform b-spline with open boundary condition

    > NOTE: useful constructor for interpolating points

    :param degree: degree of the b-spline
    :return: the open uniform b-spline
    """
    ...


@overload
def make_open_uniform(degree: int, begin: float, end: float, num_elems: int) -> BSpline:
    """Create a uniform b-spline with open boundary condition

    > NOTE: useful constructor for fitting points

    :param degree: degree of the b-spline
    :param begin: starting knot
    :param end: end knot
    :param num_elems: number of knots
    :return: the open uniform b-spline
    """
    ...


@overload
def make_open_uniform(
    degree: int, begin: float, end: float, num_elems: int, ctrl_points: npt.NDArray[np.float64] | list[float]
) -> BSpline:
    """Create a uniform b-spline with open boundary condition

    :param degree: degree of the b-spline
    :param begin: starting knot
    :param end: end knot
    :param num_elems: number of knots
    :param ctrl_points: control points
    :return: the open uniform b-spline
    """
    ...


def make_open_uniform(
    degree: int,
    begin: float | None = None,
    end: float | None = None,
    num_elems: int | None = None,
    ctrl_points: npt.NDArray[np.float64] | list[float] | None = None,
) -> BSpline:
    if all((arg is not None for arg in (begin, end, num_elems, ctrl_points))):
        return BSpline(_impl.make_open_uniform(degree, begin, end, num_elems, ctrl_points))
    if all((arg is not None for arg in (begin, end, num_elems))) and ctrl_points is None:
        return BSpline(_impl.make_open_uniform(degree, begin, end, num_elems))
    if all((arg is None for arg in (begin, end, num_elems, ctrl_points))):
        return BSpline(_impl.make_open_uniform(degree))

    error = """
    This function has three possible overloads:
    - make_open_uniform(degree)
    - make_open_uniform(degree, begin, end, num_elems)
    - make_open_uniform(degree, begin, end, num_elems, ctrl_points)
    """

    raise ValueError(dedent(error))

I quite like this to be honest. I also added a little change to how we handle input, probably having a npt.ArrayLike is the most obvious to the user, since it handles anything convertible to a np.array scalars included. I have tested rapidly and I think swig upcasts anything to a float64 so we should be good with any integral type (I tried with a int(5) and it gave me the same result as 5.0 with no warnings/errors.

[SebastianoTaddei]

Now I know that returning an array even in case of scalars may be overkill, but man does it ever simplify the code, we don't even need overloads.

[SebastianoTaddei]

Regarding overloads, it is very finicky what happens with overloaded docstrings so if they are really different I would put multiple as they do usually appear differently depending on the overload. If they are similar I wouldn't worry and put just one. Moreover, I don't know why, but when you start typing the method you will get the docstring of the last function I think, so it feels quite strange.

I pushed everything on the features/wrapper branch if you want to give it a try.

[masinag]

Yep, I like it.

Now I know that returning an array even in case of scalars may be overkill, but man does it ever simplify the code, we don't even need overloads.

I agree, in case we can add an if to check.

Regarding overloads, [...] give it a try.

I'll check later, but it's a minor thing anyway.

[SebastianoTaddei]

Top, I'll get to it tonight/tomorrow then 🚀

[masinag]

Btw I checked and the docs/autocompletion with both NeoVim and VSCode are ok. I think splitting in this way is fine.

They are suggested in the order the overloads are listed (I tried swapping them and I confirm this), and it seems a good idea to me. Choosing an appropriate order is important, since as you keep typing the next overload is suggested, if you specify more parameters.