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

ENH: linalg: Pythranized version of _solve_toeplitz.pyx #18628

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

serge-sans-paille
Copy link
Contributor

No description provided.

@serge-sans-paille serge-sans-paille force-pushed the feature/pythranize-solve-toeplitz branch 6 times, most recently from 5460af7 to 7d0cbcf Compare June 5, 2023 05:19
@serge-sans-paille serge-sans-paille changed the title [WIP] Pythranized version of _solve_toeplitz.pyx Pythranized version of _solve_toeplitz.pyx Jun 5, 2023
@serge-sans-paille
Copy link
Contributor Author

@rgommers I don't understand the remaining failure :-/

@ilayn
Copy link
Member

ilayn commented Jun 5, 2023

re-run the test, now passing

@rgommers rgommers added the Pythran Items related to internal Pythran code base label Jun 5, 2023
assert len(a) == (2*n) - 1

if a[n-1] == 0:
raise LinAlgError('Singular principal minor')
Copy link
Member

Choose a reason for hiding this comment

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

These need better error messages

scipy/linalg/_solve_toeplitz.py Outdated Show resolved Hide resolved
x_num += a[nmj] * x[j]
x_den += a[nmj] * g[m-j-1]
if x_den == 0:
raise LinAlgError('Singular principal minor')
Copy link
Member

Choose a reason for hiding this comment

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

Needs a better error message

scipy/linalg/_solve_toeplitz.py Outdated Show resolved Hide resolved
scipy/linalg/_solve_toeplitz.py Outdated Show resolved Hide resolved
scipy/linalg/_solve_toeplitz.py Show resolved Hide resolved
scipy/linalg/_solve_toeplitz.py Outdated Show resolved Hide resolved
scipy/linalg/_solve_toeplitz.py Show resolved Hide resolved
scipy/linalg/_solve_toeplitz.py Show resolved Hide resolved
@serge-sans-paille
Copy link
Contributor Author

@ilayn I did most of the requested changes, And tested the performance impact, not finding any significant changes wrt. cython implementation.

@rgommers I also added support for using blas with Pythran in meson.build, but that was my first interaction with it, so feel free to correct me.

serge-sans-paille added a commit to serge-sans-paille/pythran that referenced this pull request Jun 7, 2023
@serge-sans-paille
Copy link
Contributor Author

I've added the test case to pythran codebase to avoid regressing on it, see serge-sans-paille/pythran#2117

@serge-sans-paille
Copy link
Contributor Author

The failing check seems unrelated

@serge-sans-paille
Copy link
Contributor Author

@ilayn any though on that new revision?

@rgommers
Copy link
Member

@rgommers I also added support for using blas with Pythran in meson.build, but that was my first interaction with it, so feel free to correct me.

The PYTHRAN_BLAS_* define you changed seems to only be used for this snippet in pythran/pythonic/numpy/dot.hpp:

#ifdef PYTHRAN_BLAS_NONE
#error pythran configured without BLAS but BLAS seem needed
#endif

#if defined(PYTHRAN_BLAS_ATLAS) || defined(PYTHRAN_BLAS_SATLAS)
extern "C" {
#endif
#include <cblas.h>
#if defined(PYTHRAN_BLAS_ATLAS) || defined(PYTHRAN_BLAS_SATLAS)
}
#endif

That immediately makes a few alarm bells go off for me - we don't have any direct cblas.h includes in SciPy, and I'm almost certain it will cause problems. See, .e.g,

* This header provides numpy a consistent interface to CBLAS code. It is needed
* because not all providers of cblas provide cblas.h. For instance, MKL provides
* mkl_cblas.h and also typedefs the CBLAS_XXX enums.

@serge-sans-paille serge-sans-paille force-pushed the feature/pythranize-solve-toeplitz branch from 2277abf to 9a0923d Compare June 14, 2023 18:43
@serge-sans-paille
Copy link
Contributor Author

@rgommers for that PR, I've removed the call to @ and added a FIXME. It should not be a big deal to have pythran support 1D dot product without linking to blas, but that's on the pythran side!

@serge-sans-paille serge-sans-paille force-pushed the feature/pythranize-solve-toeplitz branch from 9a0923d to f99c9ff Compare June 14, 2023 18:46
@rgommers
Copy link
Member

Thanks, that sounds good!

@serge-sans-paille serge-sans-paille force-pushed the feature/pythranize-solve-toeplitz branch from f99c9ff to 1f44e65 Compare June 15, 2023 08:43
@serge-sans-paille
Copy link
Contributor Author

I don't understand the failure, otherwise looks good to me?

@ilayn
Copy link
Member

ilayn commented Jun 15, 2023

Nevermind that failure, it is not related but #18654 This looks good to me. But that error change might break some code somewhere. So I'm not sure about it

@serge-sans-paille serge-sans-paille force-pushed the feature/pythranize-solve-toeplitz branch 3 times, most recently from 3e482aa to 19dc998 Compare June 22, 2023 15:40
serge-sans-paille added a commit to serge-sans-paille/pythran that referenced this pull request Jun 22, 2023
@serge-sans-paille serge-sans-paille force-pushed the feature/pythranize-solve-toeplitz branch from 19dc998 to 79cb6e0 Compare June 29, 2023 08:09
@serge-sans-paille
Copy link
Contributor Author

Gentle ping :-)

@serge-sans-paille serge-sans-paille force-pushed the feature/pythranize-solve-toeplitz branch from 79cb6e0 to 51e97f8 Compare July 2, 2023 18:09
@lucascolley lucascolley added the enhancement A new feature or improvement label Dec 23, 2023
@lucascolley lucascolley changed the title Pythranized version of _solve_toeplitz.pyx ENH: linalg: Pythranized version of _solve_toeplitz.pyx Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement Pythran Items related to internal Pythran code base scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants