neurophysik / chspy Goto Github PK

In a CubicHermiteSpline, if t0 is the time of one of the anchors should CubicHermiteSpline.last_index_before(t0) return the index of that anchor or the one before t0? I'd argue the former, the current behavior is the latter.

Minimal example, following from the docs' example:

from chspy import CubicHermiteSpline
spline = CubicHermiteSpline(n=3)

#            time   state    slope
spline.add((   0 , [1,3,0], [0,1,0] ))
spline.add((   1 , [3,2,0], [0,4,0] ))
spline.add((   4 , [0,1,3], [0,4,0] ))

spline.last_index_before(4) # >> 1

my argument here being this should return 2. Granted, if you know your queried time is one of the anchors, you can just add one to the results, but if you don't know, it's more bothersome to check. The proposed change can be added by just replacing bisect_left with bisect in (this line)[https://github.com/neurophysik/CHSPy/blob/94fff4dc5f7bc1fb50b4e5b8d2d9c590a4fb641e/chspy/_chspy.py#L425].

An argument could be made for the current functionality given the name of the function: if t0 is an anchor, then you can't return the index that has t0 if the user is querying for the one before it. In that case, an alternative solution would be to add a function to cheaply check t0 in spline.times that doesn't involve (calculating the times every time)[https://github.com/neurophysik/CHSPy/blob/94fff4dc5f7bc1fb50b4e5b8d2d9c590a4fb641e/chspy/_chspy.py#L418]. If the list of times were to be saved in the instance instead of calculated every time it would become reasonable to do

if t0 in ts := spline.times:
   index = ts.index(t0)
else:
   index = spline.last_index_before(t0)

or alternatively

index = spline.last_index_before(t0)
if t0 in splines.time:
    index += 1

both these approaches would benefit from CubicHermiteSpline.times not being calculated when queried.

When messing around with this example and replacing the ChubicHermiteSpline.from_data call with a corresponding from_function call I was initially confused by the error messages, since I wasn't expecting the former to be a class method and the latter to be an instance method. Upon reading the documentation, that became clearer and I understood that I had to do

my_spline = CubicHermiteSpline(n=2)
my_spline.from_function(my_function, times)

but that begs the question: shouldn't this function be a class method to build splines from the ground up? I'm assuming the function was added with something in mind that doesn't align with this idea, and modifying it now would harm backwards compatibility. Regardless I think it might be worth sparing thought for making the API more uniform and making from_function a class method by making it detect the dimension of the spline, like from_data, or by adding an extra argument n to the method. I'd be willing to PR a draft if you don't feel you have the time to do it now, but also would like to see this implemented.