Matrix implementation part one (matrix multiplication) #77
Conversation
…o reduce number of temporary allocations.
…pile where I had enabled BLAS/LAPACK linking.
to have been initialized before we can write our code. Added small test file.
…dled in one subroutine call and not first transpose and then BLAS
…e other matrix-xx files in the future.
…ackwardsolve(), which are BLAS subroutines. This should finish basic BLAS operators
…i in something like x <- a %*% b %*% c where we have many operations on one line that each call a subroutine. We seem to have added a lot of code - I think this can be reduced.
…on are c_int and not c_ptrdiff_t
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 86.55% 88.70% +2.15%
==========================================
Files 16 17 +1
Lines 3139 3737 +598
==========================================
+ Hits 2717 3315 +598
Misses 422 422 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…small benchmarks - not much is gain over R, but still some.
Try to revert changes to compile() function to match main version and only add BLAS/LAPACK changes. Use expect_equal instead of identical to eliminate small floating point errors
|
Can't really see what I changed that makes openmp not work on macOS. Tried to make minimal changes to compile() function in R/quick.R |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…d ensureing dimensions are compatible.
|
I see we pushed two commits within a short time. I used my work computer which is windows and got codex to try and fix the problem. I see now that my runner is complete and windows clears but macOS fails. I don't know or understand why windows and macOS has such a difficult time living together. Codex had the following comments to my commit:
One thing I notice from the codex code change is that it added rtools45 - I suppose github windows is using R4.5 so this is the most relevant version and perhaps the root to the windows failure? So maybe the other changes in my commit need to be reverted to restore macOS ? I don't know. A last note: I see that the windows runner is much slower (12 vs ~18 min) after commit d7e34a8 |
|
@codex please review the PR again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f635101fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tests/testthat/helper.R
Outdated
| skip_on_cran() | ||
| skip_if_not_installed("pkgload") | ||
| if (is.null(supported)) { | ||
| supported <<- tryCatch( | ||
| { | ||
| quick(function(x) { | ||
| declare(type(x = double(1))) | ||
| declare(parallel()) | ||
| for (i in seq_len(1L)) { | ||
| x[i] <- x[i] + 1 | ||
| } | ||
| x | ||
| }) | ||
| TRUE | ||
| }, | ||
| error = function(e) { | ||
| msg <- conditionMessage(e) | ||
| if (grepl("OpenMP", msg, fixed = TRUE)) { | ||
| return(FALSE) | ||
| supported <<- { | ||
| quick(function(x) { |
There was a problem hiding this comment.
openmp_supported_or_skip() now calls quick() unconditionally and never issues a skip_if when OpenMP toolchain support is absent. On systems without OpenMP, this call raises quickr_openmp_unavailable before any skip is triggered, turning the OpenMP-specific tests into hard failures rather than optional skips. That regresses the suite on platforms without OpenMP even though the rest of quickr still works there.
Useful? React with 👍 / 👎.
|
Thank you for the PR, it looks great! The code is really solid and I appreciate the effort that went into this. I asked Codex to help with the CI/coverage issues and haven’t done a close review of those follow‑up commits yet. My plan is to carefully review the Codex changes and, once everything looks good and CI is green, I’ll be very happy to merge. Would you be open to adding a NEWS entry? This is a phenomenal feature. |
|
Thanks - though codex also deserves a lot of credit 😉. It become quite a bit larger than I expected, but it seems we need a lot of code to eliminate temporary variables and write directly to the RHS and and eliminate transpose, so we only need one function/sub-routine call. I think some of them can be consolidated so each function does a bit more. But maybe that's better left for later. You are welcome to rename the file to something different than sub-r2f-matrix. I just needed a name that was after r2f. You can also delete the doc/ before merging. But I wonder if it would be useful to have some developer documentation that could be used by an AI and humans. I at least needed some time to familiarize myself again - and honestly it is so recursive that often have difficulty imagining what is happening. |
|
And be the way, I will try to implement more BLAS/LAPACK sub routines. It will likely take some time and I guess it will be better to take one of a few functions so that the PR doesn't become overwhelming. |
|
This a great feature! Would you like to add a benchmark to README showing this off? E.g., something similar to what you showed earlier, comparing to RcppArmadillo and base R. |
|
With what we implemented here we are not quite there were we can use the example I showed earlier. If you are not planning on releasing a new version to cran soon, I think we can wait until we have implement more functions. Speaking of RcppArmadillo I remember that they also use BLAS/LAPACK and OpenMP - and it seems they also have had some troubles with getting it to work with linux/mac/win in combination. They recently tried to simplify their OpenMP detection and there is maybe something that we can learn/use. If you are interested you can perhaps read the dicussions in:
I don't know if their setup with scr/Makevar and a configure.ac file is compatible with quickr. |
|
Thanks for the links! Those will be very helpful. Regarding a CRAN release, I would like to have a CRAN release soonish. Realistically, about 2-3 weeks. The primary things I want to do before release:
After that, the next things on the horizon are:
|
2 participants
Overview
Implemented the core BLAS translation layer for matrix operations and added destination inference to minimize unnecessary temporaries. This lays the groundwork for efficient linear algebra in quickr, while documenting the planned LAPACK roadmap for solvers/decompositions.
What changed
Added BLAS-backed lowering for %*%, crossprod(), tcrossprod(), outer()/%o%, and triangular solves via forwardsolve()/backsolve() (vector + matrix RHS, transpose/upper/diag handling).
Updated the <- handler to support destination inference, so BLAS calls can write directly into the LHS when shape/type inference succeeds, avoiding intermediate buffers and extra assignments.
Tightened operand handling for matrix ops (transposed inputs, scalar/vector/matrix combinations, and clear errors for non‑conformable arguments).
The compile function now links to system BLAS/LAPACK
Docs
infer-dest-process.md documents the inference pipeline and the rules that decide when temps are created vs. reused.
baseR-blas-lapack-mapping.md provides the shared BLAS/LAPACK mapping, including what is implemented today and what’s planned next.
Tests
Expanded matrix test coverage in test-matrix-mul.R for multiply shapes, transposes, chained expressions, single‑arg crossprod/tcrossprod, outer/%o%, and triangular solves.
What’s missing / next
LAPACK-backed solvers and decompositions are still pending (e.g., solve(), chol(), qr(), svd(), eigen()), as outlined in baseR-blas-lapack-mapping.md.
Need a plan for LAPACK error handling and how errors should surface in quickr-generated code.
Need a consistent name-mangling strategy for list-style returns (e.g., eigen() values/vectors), as noted in baseR-blas-lapack-mapping.md.