-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
ENH: interpolate.BSpline: add array API standard interface #23020
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
Conversation
|
Sounds like a good idea from a high level! |
rgommers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, thanks Evgeni.
At the moment, the new attribute
xpis public, since I don't see a particularly strong need to hide it.
Private please! There is no need to leak this, even after array API support becomes fully public.
If anything, it can be useful to a user: otherwise there's no easy way for a user to tell what is the type of
spl(1)without calling it.
If this info is needed - which should be rare - isinstance on one of the array attributes is still quite cheap, and more idiomatic.
If this turns out to be problematic, we can always hide the attribute behind a property. Or store
xp.asarrayas a private method.
Renaming it from .xp to ._xp seems most straightforward here.
|
Comments addressed. Moving |
|
Good point. I think the simplest fix may be to move |
Actually, we don't if we store a private function instead of a private namespace: instead of |
j-bowhay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, not looked at the tests yet
| c = np.zeros_like(t) | ||
| c[k] = 1. | ||
|
|
||
| t, c = xp.asarray(t), xp.asarray(c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It slightly bugs me that we have to call xp.asarray on these for the sole purpose of being able to get the array namespace later but I guess the cost of doing so is pretty low
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bugs me too, a bit. This method is not meant to be in a tight loop though, and the parts that are, have numpy arrays to work on.
| c = self.c.copy() | ||
| c = self._asarray(self.c, copy=True) | ||
| t = self.t | ||
| xp = array_namespace(t, c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not store xp when the object is initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf.
| self._xp = xp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's deliberate, actually:
lorentzenchr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if c and t are different arrays like BSpline(t=torch_array, c=numpy_array, 2)? (Not that it makes sense, but in case a user does it.)
|
convert BSpline (minus design_matrix, which returns a csr_array), splder, splantider, make_interp_spline Since heavy lifting is in C, only do array_namespace on inputs, convert to numpy, do the work, convert to self.xp on exit.
xp.concat does not accept python scalars or 0D arrays, so we need a helper for thing like `np.r_[0, x]` and `np.r_[x[0], x]`.
Store a private `_asarray` function on a BSpline instance instead of the namespace. This way, we do not need a custom getstate/setstate or reduce or any other custom pickle support.
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
What Lucas said: mixing array types in the constructor is an automatic TypeError from In this PR --- and I think this is correct in general --- the logic is that the array namespace is fixed by the arguments of the constructor; at call time, the arguments are cast to the array type of that fixed namespace: |
|
Addressed all review comments. |
Maybe it does not matter much and consistency within scipy is more import, but as a user I would expect to get the same array back as I pass into |
lucascolley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Evgeni, very close!
| xp = array_namespace(t) | ||
| t = np.asarray(t) | ||
| k = t.shape[0] - 2 | ||
| t = _as_float_array(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe consider renaming _as_float_array? It was not clear to me that it was something NumPy-specific. perhaps include 'c_contiguous'?
What is the precise reason we need to convert t to and from np in this method? Can the np.r_ call not be replaced with concat_1d?
If we only need to worry about C-contiguity for NumPy arrays, could we not make _as_float_array operate on xp arrays by using _array_api._asarray(..., order='C')?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No opinion on renaming _as_float_array. Send a patch if you have feel strongly about it?
Conversions to/from numpy: there are of course multiple ways to skin this one. It's used ten times,
$ git grep -n "_as_float_array" scipy/interpolate/ | wc -l
11
and IIRC other uses are after the conversion to numpy. So it felt easier to just do it like it's done here and be done with it. Care to send a patch if you see a way to make it nicer across the module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, the only reason we are moving to np here is to use _as_float_array? If so, that seems bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ATM all alternative constructors, including basis_element, use this pattern. If a clean-up is desired, we can do it in a sequel, after #23052 which converts other users of _as_float_array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, if you add a TODO comment here to avoid the namespace conversion I am happy to merge!
Alternatively, I could try to come up with a patch in the coming days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A patch would be great indeed @lucascolley! Meanwhile I'll then add a TODO comment to gh-23052 to avoid flushing the CI here.
| check_splev(b1, j, der, 1e-12, 1e-12) | ||
|
|
||
|
|
||
| ### stolen from @pv, verbatim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
lucascolley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Evgeni, and for the review @lorentzenchr !
|
Merging as approved. Thanks to all reviewers! |
This PR adds a minimal Array API interface to
BSplineandmake_interp_spline.There are several minor details worth spelling out. First,
BSplineuses a C extension, therefore non-numpy array inputs need to go through the now-standardnp.asarray(input); result = c_function(input); return xp.asarray(result)dance.Second,
BSplineinstances used to store numpy arrays. Namely, for array-liketandc, andspl = BSpline(t, c, k), thenspl.tandspl.care numpy array attributes derived from the__init__arguments.This PR changes these attributes into properties, so that if
tandcare torch tensors, andspl = BSpline(t, c, k),a new private attribute
spl._tis a numpy array, andspl.tis a propery which constructs a torch tensor fromspl._t.The advantage of this is that at the call time, we can use
spl._twithout paying the price ofnp.asarray(which is not negligible here, actually). An unpleasant consequence is that the property abstraction leaks for mutable properties:spl.t.fill(0)goes through the getter not the setter. There's nothing to be done about it though, and this does not lead to new failure modes AFAICS.Third, if
t,candxare torch tensors, thenspl = BSpline(t, c, k); spl(x)should produce a torch tensor, too. To achieve this, we store the namespace on an instance, viaself.xp = array_namespace(t, c)and the__call__isAt the moment, the new attribute
xpis public, since I don't see a particularly strong need to hide it. If anything, it can be useful to a user: otherwise there's no easy way for a user to tell what is the type ofspl(1)without calling it.If this turns out to be problematic, we can always hide the attribute behind a property. Or store
xp.asarrayas a private method.One last small thing: since module objects are not picklable, we need to add a custom pickler to correctly handle the
xpattribute.