Skip to content

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented Sep 10, 2024

A follow-up to #21223, chipping small bits and pieces from a cython extension: move the _handle_lhs_derivatives helper from Cython to python. The code is basically pure numpy, and the computational complexity is O(1), so there is no reason for this helper to stay in Cython or move to C++.

The computational complexity is O(1), so there is no reason
for this helper to stay in Cython or move to C++.
@ev-br ev-br added scipy.interpolate maintenance Items related to regular maintenance tasks labels Sep 10, 2024
@github-actions github-actions bot added the Cython Issues with the internal Cython code base label Sep 10, 2024
double[::1] wrk = np.empty(2*k+2, dtype=np.float64)

# derivatives @ xval
with nogil:
Copy link
Member

Choose a reason for hiding this comment

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

just for my understanding, could you explain the relationship between this with nogil code and the Python translation?

Copy link
Member Author

@ev-br ev-br Sep 10, 2024

Choose a reason for hiding this comment

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

The code is precisely the same, modulo that in python it operates on numpy arrays and in cython it was taking typed memoryviews of arrays and operating on those.

And while it was in a with nogil section, the is doing so little work that releasing the GIL is very unlikely to have any effect.

Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

LGTM!

@lucascolley lucascolley added this to the 1.15.0 milestone Sep 10, 2024
# compute and fill in the derivatives @ xval
for row in range(deriv_ords.shape[0]):
nu = deriv_ords[row]
wrk = _bspl.evaluate_all_bspl(t, k, xval, left, nu)
Copy link
Contributor

Choose a reason for hiding this comment

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

I did notice the use of this function instead of _deBoor_D directly, but looks like that still gets called internally, so I don't see any obvious issues on my end either, and CI is happy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, evaluale_all_bspl is a python-side wrapper of C-only _deBoor_D. Both routines are non-public, so can probably unify the naming. Will think about it in the ongoing refactor of the internals (I've a branch, not PR-ready yet)

@tylerjereddy tylerjereddy merged commit 46344fe into scipy:main Sep 10, 2024
40 checks passed
@tylerjereddy
Copy link
Contributor

Thanks Evgeni, Lucas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython Issues with the internal Cython code base maintenance Items related to regular maintenance tasks scipy.interpolate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants