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

FastLAPACK #153

Merged
merged 6 commits into from
Jul 13, 2022
Merged

FastLAPACK #153

merged 6 commits into from
Jul 13, 2022

Conversation

rayegun
Copy link
Collaborator

@rayegun rayegun commented Jul 13, 2022

This is just a basic set of functions I was able/willing to develop on my laptop at sea. I've only tested A*x \approx b for those factorizations right now.

I'll add tests shortly. A few questions:

  1. Should we default to this workspace system for QR and LU? Rather than having "FastQRFactorization"
  2. Should the workspace be resized when a user uses set_A with a different size matrix? Problematically I triggered a segfault once when playing with the sizing of A this evening, so I'll have to track that down. It's supposed to just error (and mostly does). FastLAPACKInterface.jl doesn't provide a method to resize workspaces as far as I can tell.

@codecov
Copy link

codecov bot commented Jul 13, 2022

Codecov Report

Merging #153 (5854ef0) into main (e4e450d) will decrease coverage by 4.51%.
The diff coverage is 0.00%.

❗ Current head 5854ef0 differs from pull request most recent head 44a9a33. Consider uploading reports for the commit 44a9a33 to get more accurate results

@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
- Coverage   65.28%   60.77%   -4.52%     
==========================================
  Files           9        9              
  Lines         579      622      +43     
==========================================
  Hits          378      378              
- Misses        201      244      +43     
Impacted Files Coverage Δ
src/LinearSolve.jl 75.00% <ø> (ø)
src/factorization.jl 62.68% <0.00%> (-17.07%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ChrisRackauckas
Copy link
Member

Should we default to this workspace system for QR and LU? Rather than having "FastQRFactorization"

I think the high level types is probably easiest.

Should the workspace be resized when a user uses set_A with a different size matrix? Problematically I triggered a segfault once when playing with the sizing of A this evening, so I'll have to track that down. It's supposed to just error (and mostly does). FastLAPACKInterface.jl doesn't provide a method to resize workspaces as far as I can tell.

That would be cool if possible.

@ChrisRackauckas
Copy link
Member

Now for the real test: does it actually end up faster than direct OpenBLAS/MKL?

@rayegun
Copy link
Collaborator Author

rayegun commented Jul 13, 2022

Well it just avoids allocations right? That's the only change AFAICT. So it should be fast for resolves

@ChrisRackauckas
Copy link
Member

Run the formatter?

@ChrisRackauckas
Copy link
Member

Actually, I'll run the formatter. I want to update the docs anyways.

@rayegun
Copy link
Collaborator Author

rayegun commented Jul 13, 2022

Oh oops. @ChrisRackauckas just formatted and added a simple set of tests.

@ChrisRackauckas
Copy link
Member

oh that's more than fine.

@ChrisRackauckas ChrisRackauckas merged commit aeedba4 into SciML:main Jul 13, 2022
Will Kimmerer and others added 2 commits July 13, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants