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: Tiled QR wrappers for scipy.linalg.lapack #10345

Merged
merged 10 commits into from Jul 2, 2019
Merged

ENH: Tiled QR wrappers for scipy.linalg.lapack #10345

merged 10 commits into from Jul 2, 2019

Conversation

AidanGG
Copy link
Contributor

@AidanGG AidanGG commented Jun 24, 2019

As per #10323 I have begun working on the wrappers. I believe the wrappers and documentation are complete.

I still need to write some tests (but they do appear to function according to my brief, informal testing).

Closes #10323

@AidanGG
Copy link
Contributor Author

AidanGG commented Jun 25, 2019

@ilayn I believe this is ready to be looked at, but there are some CI issues with the documentation? How would I fix this?

@AidanGG AidanGG changed the title [WIP] Tiled QR wrappers for scipy.linalg.lapack Tiled QR wrappers for scipy.linalg.lapack Jun 25, 2019
@ilayn
Copy link
Member

ilayn commented Jun 26, 2019

Great. Thanks for the time and effort. I will check further today more carefully.

One of the travis failures is real though for float32 dtype. The fft stuff you can ignore for now; it is a new feature and CIs didn't refreshed apparently

@AidanGG
Copy link
Contributor Author

AidanGG commented Jun 26, 2019

Thanks, my test seems to fail on Travis with Python 3.5. I will investigate this locally with a new virtualenv.

In the mean time, I've pushed a change that adds a depends() to the f2py wrapper that might fix it.

Edit: This appears to have fixed it.

@ilayn ilayn changed the title Tiled QR wrappers for scipy.linalg.lapack ENH: Tiled QR wrappers for scipy.linalg.lapack Jun 26, 2019
Copy link
Member

@ilayn ilayn left a comment

Choose a reason for hiding this comment

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

Other than the minor issue, I don't have any other issues.

scipy/linalg/flapack_other.pyf.src Outdated Show resolved Hide resolved
scipy/linalg/flapack_other.pyf.src Outdated Show resolved Hide resolved
@ilayn
Copy link
Member

ilayn commented Jun 27, 2019

The CircleCI stuff is irrelevant but in Travis run I think you are hitting the same issue that I had in #8965

The documentation is getting choked on the first unmatched * character of the size while trying to write the parameter bounds. You can use side[0] instead to escape from it.

@AidanGG
Copy link
Contributor Author

AidanGG commented Jun 27, 2019

Alright, these changes appear to have fixed the docs building.

@ilayn
Copy link
Member

ilayn commented Jun 27, 2019

Thanks! Now it looks good to me, let's wait for a while in case others want to chime in.

@ilayn ilayn added enhancement A new feature or improvement scipy.linalg labels Jun 27, 2019
@ilayn ilayn added this to the 1.4.0 milestone Jun 27, 2019
@ilayn
Copy link
Member

ilayn commented Jul 2, 2019

OK let's push this in. Since this is for 1.4 we would still have time to fix it in case I missed a detail. Thanks again @AidanGG, Keep them coming!

And then the obligatory copy/paste response:

Just for the future reference, please make separate branches to work on new features and to send PRs such that your standard working repo does not interfere with the PRs you have submitted. Once a PR is merged you can safely delete that branch and keep working on other branches.

@ilayn ilayn merged commit df6b808 into scipy:master Jul 2, 2019
@AidanGG
Copy link
Contributor Author

AidanGG commented Jul 2, 2019

Thanks, @ilayn. I'm thinking about adding some of the routines necessary for tiled Cholesky, which I'll get around to doing once I need them for my own work.

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 scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding more low-level LAPACK wrappers
2 participants