Matrix part 2 - #79
Conversation
…r_matrix that handles vector-like matrices (1x1, 1xn, nx1) and elementwise operator with vectors
…cess functions like t(x)[5] , crossprod(x)[1] and related.
|
@codex please review. Do not flag issues with not handling info from LAPACK calls - this is well known and left for later. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 88.11% 89.35% +1.23%
==========================================
Files 22 22
Lines 3989 4442 +453
==========================================
+ Hits 3515 3969 +454
+ Misses 474 473 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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.
Reviewed commit: ae93bbe263
ℹ️ 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".
Add dest_infer support for diag() to eliminate unnecessary temporary matrix allocations. Previously, `out <- diag(x, nrow, ncol)` would allocate a temporary matrix, fill it, then copy to out. Now it writes directly to the destination. Also fix argument extraction to support both named and positional args: - diag(): was ignoring positional nrow/ncol args (e.g., `diag(x, 5, 7)`) - crossprod/tcrossprod: add support for named x/y args - forwardsolve/backsolve: add support for named l/r/x args - chol/chol2inv: add support for named x arg This ensures handlers and inference functions match R's function signatures and work correctly regardless of calling convention.
Consolidate duplicate conformability checking code and add tests for inference NULL paths and LAPACK error handling.
…y remove many if() with assert functions.
|
I created this script to help me get the test coverage up. Maybe you have a way to get the data from codecov, but I couldn't so I created this script and thought it might be useful to you too library(httr2)
library(jsonlite)
get_codecov_report <- function(token = NULL, pull_id = NULL) {
if(any(is.null(token) || is.null(pull_id))) stop("token and pull_id must be set")
service <- "github"
owner <- "t-kalinowski"
repo <- "quickr"
compare_url <- glue::glue("https://api.codecov.io/api/v2/{service}/{owner}/repos/{repo}/compare/")
request(compare_url) |>
req_url_query(pullid = pull_id) |>
req_headers(Authorization = paste("Bearer", token)) |>
req_perform() |>
resp_body_json()
}
parse_report <- function(diff_report) {
out <- list()
out$commit_ids <- list(
base_commit = diff_report$base_commit,
head_commit = diff_report$head_commit
)
out$pr_stats <- list(
base = list(
lines_covered = diff_report$totals$base$hits,
lines_not_covered = diff_report$totals$base$misses,
pr_coverage = diff_report$totals$base$coverage
),
head = list(
lines_covered = diff_report$totals$head$hits,
lines_not_covered = diff_report$totals$head$misses,
pr_coverage = diff_report$totals$head$coverage
),
delta = list(
lines_covered = diff_report$totals$head$hits - diff_report$totals$base$hits,
lines_not_covered = diff_report$totals$head$misses - diff_report$totals$base$misses,
pr_coverage = diff_report$totals$head$coverage - diff_report$totals$base$coverage
)
)
files <- diff_report$files
changed_files <- list()
for(file in files) {
if(!file$has_diff) next
changed_file <- list()
changed_file$base_and_head_file_names <- list(
base_file_name = file$name$base,
head_file_name = file$name$head
)
changed_file$file_stats <- list(
base = list(
lines_covered = file$totals$base$hits,
lines_not_covered = file$totals$base$misses,
pr_coverage = file$totals$base$coverage
),
head = list(
lines_covered = file$totals$head$hits,
lines_not_covered = file$totals$head$misses,
pr_coverage = file$totals$head$coverage
),
delta = list(
lines_covered = file$totals$head$hits - file$totals$base$hits,
lines_not_covered = file$totals$head$misses - file$totals$base$misses,
pr_coverage = file$totals$head$coverage - file$totals$base$coverage
)
)
lines <- file$lines
changed_lines <- list()
idx_line <- 0
for(line in lines) {
line_coverage <- line$coverage$head %||% 0
if(line_coverage != 1) next
idx_line <- idx_line + 1
line_info <- list(
uncoverd_code = line$value,
line_number = line$number$head,
is_new_code = line$added,
diff_from_base = line$is_diff
)
changed_lines[[idx_line]] <- line_info
}
file_name <- file$name$head
changed_files[[file_name]] <- changed_file
changed_files[[file_name]]$changed_lines <- changed_lines
}
out$changed_files <- changed_files
out
}
token <- Sys.getenv("CODECOV_QUICKR")
pull_id <- 79
output_file <- glue::glue("scratch/codecov/PR/{pull_id}/codecov_report.json")
dir.create(dirname(output_file), recursive = TRUE, showWarnings = FALSE)
get_codecov_report(token, pull_id) |>
parse_report() |>
toJSON(pretty = T, auto_unbox = T) |>
writeLines(output_file) |
|
I kind of started it in the last commit, but I am thinking about making a rather big refactor off the matrix files. There are a lot of small I think the current PR is reasonable complete with a high code coverage, so maybe you want to review as is or wait for a refactor? |
|
@codex do a final review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32b44c700c
ℹ️ 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".
…press warning in order to not fail tests
|
Added to news.md and also an example to readme.md. I created a new example for RcppArmadillo that mimics the R code better and they are now about the same in performance. I might be a good idea for you to render the readme so it is more consistent? |
|
Thank you for the excellent PR! I'm will merge this as is, and then handle the tiny handful of nits/style-preferences I have in a subsequent PR. This is great! |
|
@t-kalinowski thanks. Just for your information: Also, for some reason it seems that benchmarks (for me at leat) runs slower in a rmarkdown/knitr context compared to console/terminal - but maybe that is expected. |
2 participants
Second batch of functions. These functions enable us to do linear regression with the example in the original issue.
See the doc/solve... file to see some ekstra context on changes to r2f.R (in the bottom of the document.)
Matrix LAPACK Handlers: Implementation Walkthrough
This document explains the changes made to support
solve(),chol(),chol2inv(), anddiag()along with the linear-model example.1) Add dimension expression support
R/manifest.R: addedmin()andmax()to the dimension expressionevaluator so Fortran declarations can emit
min(a, b)andmax(a, b)instead of failing on those calls.
2) New BLAS/LAPACK emitters
R/r2f-matrix-blas.R: introduced LAPACK helpers and diag utilities:lapack_solve()usesdgesvto solveA %*% x = Band respectsdestination inference while avoiding destructive overwrite of inputs.
lapack_inverse()usesdgetrf+dgetriforsolve(A).lapack_chol()usesdpotrfand zeroes the lower triangle to matchbase R
chol()output.lapack_chol2inv()usesdpotriand mirrors the upper triangle toproduce a full symmetric inverse.
diag_extract()anddiag_matrix()cover extraction and constructionforms for
diag().diag_length_expr()computesmin(nrow, ncol)while keeping theexpression symbolic when needed.
zero_lower_triangle()mirrors base R output by clearing the lowertriangle after
dpotrf.Updated
blas_int()to safely deparse language inputs so expressions likeint(min(a, b), kind=c_int)render correctly.Note: LAPACK status (
info) is not propagated yet, so singular ornon-PD inputs will currently produce undefined numeric results instead of
R errors. This will be revisited once we have a reliable mechanism to
bubble Fortran failures back to R.
3) Destination inference for new handlers
R/r2f-matrix-infer.R: added inference helpers for:solve()(vector, matrix, and inverse cases)chol()andchol2inv()This allows the assignment handler to pass a destination and avoid extra
allocations when possible.
4) Register new matrix handlers
R/r2f-matrix.R:solve(),chol(),chol2inv(), anddiag().pivot=TRUEforchol) are rejectedwith clear messages.
5) Vector and scalar reshape behavior
R/r2f.R:maybe_reshape_vector_matrix()andscalarize_matrix()soelementwise operations can handle vector vs 1x1 matrix and vector vs
matrix with singleton dimensions.
scalarize_matrix()(they become ascalar designator like
x(1, 1)), which keeps Fortran ranks consistent.dimensions when needed.
diag(XtX_inv) * s2whens2is a 1x1 matrix from
crossprod(res)anddiag()returns a vector.hoistthrough[lowering so temporary arrays created byBLAS/LAPACK helpers (e.g.,
crossprod(res)emitting a temp insyrk())are declared and in scope when immediately subscripted like
crossprod(res)[1].This matters because Fortran cannot subscript arbitrary expressions, so
hoisting ensures the temp has a name and valid storage before
[...].6) Tests added
tests/testthat/test-matrix-lapack.R:solve()vector, matrix, and inverse.chol()andchol2inv().diag()for vector input, matrix extraction, and size-based construction.7) Commands executed
air format .R -q -e 'devtools::test()'R -q -e 'rcmdcheck::rcmdcheck(error_on = "warning")'All tests and
R CMD checkcompleted without errors or warnings.