Lower array(data=<non-scalar>, dim=...) via reshape() - #93
Conversation
Lower array(data=<non-scalar>, dim=...) to Fortran reshape() so rank-N casts can restore the original shape. Add a regression test covering array(as.double(x), dim=...) for a rank-3 input.
![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: 66ef931ae9
ℹ️ 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".
| if (is.null(dim_vec@value) || dim_vec@value@mode != "integer") { | ||
| stop("array(dim=) must be an integer vector", call. = FALSE) | ||
| } | ||
| if (dim_vec@value@rank != 1L) { | ||
| stop("array(dim=) must be a 1-d integer vector", call. = FALSE) |
There was a problem hiding this comment.
The new validation rejects any dim that isn't an integer vector with rank 1, but base R allows numeric (double) dims and also allows a scalar dim (length‑1) which is coerced to an integer vector. This means common calls like array(as.double(x), dim = c(2, 3)) or array(as.double(x), dim = 2) will now error at compile time even though int() would coerce successfully. Consider relaxing the check to accept numeric/scalar dims and coercing to integer rather than stopping.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
=======================================
Coverage 92.78% 92.78%
=======================================
Files 27 27
Lines 5749 5749
=======================================
Hits 5334 5334
Misses 415 415 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Ensure Fortran RESHAPE() receives an array-valued SOURCE; expand scalar-fill constructors (integer/double/logical) to the required element count. - Derive SHAPE from r2dims()+dims2f() so dim can be supplied as an AST literal vector. - Add regression tests covering both cases.
- Teach array() reshape to interpret dim=2:4 and dim=d (where d is bound to a literal sequence) as compile-time dims. - In fill-constructor reshape, compute element count from per-axis dims2f() results instead of splitting dims2f() output on commas (dims can include calls like size(x, 1)). - Add regression tests covering both cases.
- Add regression for array(<non-scalar>, dim=1) which previously produced rank-mismatched RESHAPE() assignments. - In array() lowering, special-case scalar-like target dims (dim=1) to extract the first element via a hoisted temp array, avoiding RESHAPE() returning rank-1.
- Add regression test for array(data=<expr requiring hoist>, dim=...) to ensure hoisted temporaries are emitted as statements (not embedded in RESHAPE source). - Forward hoist explicitly in array() and matrix() handlers now that array() has an explicit hoist parameter.
Add regression test for dim=c() and fail early with a clear error, matching base R behavior instead of emitting rank-mismatched Fortran.
1 participant
Problem
quickr currently rejects
array()whendatais not scalar, which breaks the common pattern used to restore dimensions after a vectorized cast (e.g.array(as.double(x), dim = shape_in)).This shows up as a compile-time error:
array(data=) must be a scalar for nowChange
array()lowering to accept non-scalardataby emitting Fortranreshape(data, int(dim))(no recycling).Test
array(as.double(x), dim = c(2L, 3L, 4L))and checksexpect_quick_identical()against R.