Implement uniform normal and integers distributions - #257
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds three public random tensor generators in mohu-random: ChangesRandom Tensor Sampling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/mohu-random/src/continuous.rs (1)
23-24: ⚡ Quick winAdd error context to propagated
?.
Buffer::from_vecandreshapeerrors propagate without describing what was attempted. Per convention, attach context (e.g..with_context(|| ...)). Same pattern applies tonormal(Lines 51-52) andintegersindiscrete.rs.♻️ Example
- let buf = Buffer::from_vec(data)?; - buf.reshape(shape) + let buf = Buffer::from_vec(data).context("building uniform sample buffer")?; + buf.reshape(shape).context("reshaping uniform samples to target shape")As per coding guidelines: "Wrap error context with
.context(...)or.with_context(|| ...)".🤖 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-random/src/continuous.rs` around lines 23 - 24, The Buffer::from_vec(...) and buf.reshape(...) calls currently propagate errors with ? and need contextualized errors; update the code in continuous.rs to call Buffer::from_vec(data).with_context(|| "creating Buffer from data for <function/context name>")? and then buf.reshape(shape).with_context(|| format!("reshaping Buffer to {:?} in <function/context name>", shape))? (replace <function/context name> with the containing function like `sample` or the public API name), and apply the same pattern to the `normal` implementation (the two calls around lines 51-52) and to `integers` in discrete.rs—use .context(...) or .with_context(|| ...) on each fallible call so logs show what operation failed and include identifying info (function name and parameters) in the message.
🤖 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-random/src/continuous.rs`:
- Around line 7-13: The uniform function currently only checks high <= low,
which misses NaN/inf cases; update Mohu-random's uniform(shape: &[usize], low:
f64, high: f64) to also validate that both low.is_finite() and high.is_finite()
are true (and still enforce high > low), returning MohuError::domain("uniform",
"...") with an appropriate message when they are not finite or when high <= low,
before calling random_range(low..high).
- Around line 40-47: The Box–Muller implementation can produce inf/NaN when u1
== 0.0; change how u1 is drawn so it lies in (0,1] instead of [0,1). Replace the
current `let u1: f64 = rng.random();` with a non-zero mapping such as `let u1:
f64 = 1.0 - rng.random();` (or loop/retry if zero) so `u1.ln()` is safe; keep
the rest of the `z0` computation and returned expression (`z0`, `u2`, `mean +
std * z0`) unchanged.
- Around line 8-13: Replace the manual bound-check returns with the ensure!
macro: for continuous::uniform, swap the if high <= low { return
Err(MohuError::domain("uniform", "high must be greater than low")); } with
ensure!(high > low, MohuError::domain("uniform", "high must be greater than
low")); likewise update the similar validation in continuous::normal and
discrete::integers to use ensure!(cond, MohuError::domain(...)) with the
identical domain strings and messages so the same error values are produced.
In `@crates/mohu-random/src/discrete.rs`:
- Around line 7-24: Replace the explicit return Err(...) check in integers with
the project's ensure! convention (ensure!(high > low,
MohuError::domain("integers", "high must be greater than low"))), and propagate
errors from Buffer::from_vec(data) and buf.reshape(shape) with contextual
messages instead of plain ?; i.e., call
Buffer::from_vec(data).context("integers: failed to create buffer from vec")?
and then buf.reshape(shape).context("integers: failed to reshape buffer")? so
both failure points include the "integers" context and follow the same
error-context conventions used in continuous.rs.
---
Nitpick comments:
In `@crates/mohu-random/src/continuous.rs`:
- Around line 23-24: The Buffer::from_vec(...) and buf.reshape(...) calls
currently propagate errors with ? and need contextualized errors; update the
code in continuous.rs to call Buffer::from_vec(data).with_context(|| "creating
Buffer from data for <function/context name>")? and then
buf.reshape(shape).with_context(|| format!("reshaping Buffer to {:?} in
<function/context name>", shape))? (replace <function/context name> with the
containing function like `sample` or the public API name), and apply the same
pattern to the `normal` implementation (the two calls around lines 51-52) and to
`integers` in discrete.rs—use .context(...) or .with_context(|| ...) on each
fallible call so logs show what operation failed and include identifying info
(function name and parameters) in the message.
🪄 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: bbaedce0-a012-40a0-bcc3-9b2d165bd9b5
📒 Files selected for processing (2)
crates/mohu-random/src/continuous.rscrates/mohu-random/src/discrete.rs
| pub fn integers(shape: &[usize], low: i64, high: i64) -> MohuResult<Buffer> { | ||
| if high <= low { | ||
| return Err(MohuError::domain( | ||
| "integers", | ||
| "high must be greater than low", | ||
| )); | ||
| } | ||
|
|
||
| let n: usize = shape.iter().product(); | ||
|
|
||
| let mut rng = rand::rng(); | ||
|
|
||
| let data: Vec<i64> = (0..n) | ||
| .map(|_| rng.random_range(low..high)) | ||
| .collect(); | ||
|
|
||
| let buf = Buffer::from_vec(data)?; | ||
| buf.reshape(shape) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Same ensure! and error-context conventions apply here.
The return Err(...) at Lines 8-13 should use ensure!, and the ? propagation at Lines 23-24 should add .context(...), as detailed in the continuous.rs comments. Logic (exclusive high, i64 range sampling) is otherwise correct.
🤖 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-random/src/discrete.rs` around lines 7 - 24, Replace the explicit
return Err(...) check in integers with the project's ensure! convention
(ensure!(high > low, MohuError::domain("integers", "high must be greater than
low"))), and propagate errors from Buffer::from_vec(data) and buf.reshape(shape)
with contextual messages instead of plain ?; i.e., call
Buffer::from_vec(data).context("integers: failed to create buffer from vec")?
and then buf.reshape(shape).context("integers: failed to reshape buffer")? so
both failure points include the "integers" context and follow the same
error-context conventions used in continuous.rs.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
5c6a164 to
f7099b6
Compare
Signed-off-by: Vishakha Ojha <vishakhaojha57@gmail.com>
Signed-off-by: Vishakha Ojha <vishakhaojha57@gmail.com>
f7099b6 to
36b223f
Compare
2 participants
What
Why
How
Checklist
cargo test --workspacepassescargo clippy --workspace -- -D warningspassescargo fmt --allappliedCHANGELOG.mdupdated (if user-facing change)Summary by CodeRabbit