feat(mohu-ops): implement matrix multiplication - #299
Conversation
PR Check Summary
CI will run: build, test, clippy, fmt, cargo-deny, DCO, semver. |
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds and exports ChangesMatrix multiplication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant matmul
participant check_k
participant matmul_typed
participant Buffer
Caller->>matmul: Provide lhs and rhs buffers
matmul->>check_k: Validate shared dimension
matmul->>Buffer: Cast operands and allocate output
matmul->>matmul_typed: Dispatch typed kernel
matmul_typed-->>matmul: Write computed values
matmul-->>Caller: Return output Buffer
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0f91ebb to
ae5aef7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/mohu-ops/src/matmul.rs (1)
10-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublic API missing doc comment.
matmulis exported viapub mod matmul;inlib.rsand is the crate's headline public entry point (per issue#152), but has no doc comment. Consider adding one describing supported shape combinations, promotion rules, and error conditions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mohu-ops/src/matmul.rs` around lines 10 - 16, Add a Rust doc comment to the public matmul function describing supported input shape combinations, how lower-rank operands are promoted, and the shape or validation errors it returns. Keep the documentation attached directly to matmul and aligned with its implemented behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mohu-ops/tests/matmul_tests.rs`:
- Line 22: Add mohu-testing = { workspace = true } to the [dev-dependencies] in
crates/mohu-ops/Cargo.toml, then update all four float assertions in
crates/mohu-ops/tests/matmul_tests.rs (lines 22, 51, 69, and 87) to use
mohu_testing::approx::assert_allclose instead of assert_eq!, preserving each
expected value slice.
---
Nitpick comments:
In `@crates/mohu-ops/src/matmul.rs`:
- Around line 10-16: Add a Rust doc comment to the public matmul function
describing supported input shape combinations, how lower-rank operands are
promoted, and the shape or validation errors it returns. Keep the documentation
attached directly to matmul and aligned with its implemented behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e9dd5d3-53ad-47ab-a319-c38f4c45793a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/mohu-ops/Cargo.tomlcrates/mohu-ops/src/lib.rscrates/mohu-ops/src/matmul.rscrates/mohu-ops/tests/matmul_tests.rs
|
|
||
| let out = c.as_slice::<f32>().unwrap(); | ||
|
|
||
| assert_eq!(out, &[19.0, 22.0, 43.0, 50.0,]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Wire up mohu-testing and use assert_allclose instead of assert_eq! on floats. The four value-checking matmul tests compare f32 slices with assert_eq!, and no mohu-testing dev-dependency is present to support the required helper — same root cause across both files.
crates/mohu-ops/tests/matmul_tests.rs#L22-L22: replaceassert_eq!(out, &[19.0, 22.0, 43.0, 50.0,])withmohu_testing::approx::assert_allclose.crates/mohu-ops/Cargo.toml#L15-L24: addmohu-testing = { workspace = true }as a[dev-dependencies]entry scoped to this crate.crates/mohu-ops/tests/matmul_tests.rs#L51-L51: replaceassert_eq!(out, &[32.0])withassert_allclose.crates/mohu-ops/tests/matmul_tests.rs#L69-L69: replaceassert_eq!(out, &[13.0, 16.0])withassert_allclose.crates/mohu-ops/tests/matmul_tests.rs#L87-L87: replaceassert_eq!(out, &[17.0, 39.0])withassert_allclose.
🧪 Proposed fix
- assert_eq!(out, &[19.0, 22.0, 43.0, 50.0,]);
+ mohu_testing::approx::assert_allclose(out, &[19.0, 22.0, 43.0, 50.0]);📍 Affects 2 files
crates/mohu-ops/tests/matmul_tests.rs#L22-L22(this comment)crates/mohu-ops/Cargo.toml#L15-L24crates/mohu-ops/tests/matmul_tests.rs#L51-L51crates/mohu-ops/tests/matmul_tests.rs#L69-L69crates/mohu-ops/tests/matmul_tests.rs#L87-L87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mohu-ops/tests/matmul_tests.rs` at line 22, Add mohu-testing = {
workspace = true } to the [dev-dependencies] in crates/mohu-ops/Cargo.toml, then
update all four float assertions in crates/mohu-ops/tests/matmul_tests.rs (lines
22, 51, 69, and 87) to use mohu_testing::approx::assert_allclose instead of
assert_eq!, preserving each expected value slice.
Source: Coding guidelines
d4587f0 to
618d02a
Compare
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Gayatri Padalia <gayatripadalia@gmail.com> Signed-off-by: gayatripadalia <gayatripadalia@gmail.com>
Signed-off-by: gayatripadalia <gayatripadalia@gmail.com>
Signed-off-by: gayatripadalia <gayatripadalia@gmail.com>
Signed-off-by: gayatripadalia <gayatripadalia@gmail.com>
e013cad to
0fcb76c
Compare
2 participants
What
Implements matrix multiplication for
mohu-opsby adding a newmatmuloperation that supports the following cases:The implementation performs shape validation, dispatches over supported numeric dtypes, promotes the output dtype appropriately, and returns errors for incompatible shapes or unsupported ranks.
Additionally, this PR adds unit tests covering the supported multiplication variants, shape mismatch handling, and integer-to-
f64promotion.Why
Matrix multiplication is a fundamental linear algebra primitive and is required for many higher-level numerical operations. This PR addresses the implementation requested in issue #152.
Closes #152.
How
Added
crates/mohu-ops/src/matmul.rscontaining the matrix multiplication implementation.Exported the new module from
mohu-ops.Used runtime numeric dtype dispatch to invoke the appropriate typed kernel while rejecting unsupported dtypes.
Implemented validation for supported input ranks (1D and 2D) and compatible inner dimensions.
Added unit tests covering:
f64Checklist
cargo test --workspacepassescargo clippy --workspace -- -D warningspassescargo fmt --allappliedCHANGELOG.mdupdated (not required; internal API addition)##SS
Summary by CodeRabbit
matmul, including matrix×matrix, matrix×vector, vector×matrix, and dot-product behavior with correct output shape handling.