Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 113 additions & 1 deletion crates/mohu-index/src/boolean.rs
Original file line number Diff line number Diff line change
@@ -1 +1,113 @@
// boolean — implementation pending
use mohu_buffer::buffer::Buffer;
use mohu_buffer::strides::NdIndexIter;
use mohu_dtype::dispatch_dtype;
use mohu_dtype::dtype::DType;
use mohu_error::{MohuError, MohuResult};

/// Boolean mask indexing.
///
/// Returns a new 1D buffer containing only the elements of `src` where the
/// corresponding element of `mask` is `true`.
///
/// # Errors
///
/// - `DomainError` if `mask.dtype() != DType::Bool`
/// - `BoolIndexShapeMismatch` if `mask.shape() != src.shape()`
pub fn index_bool(src: &Buffer, mask: &Buffer) -> MohuResult<Buffer> {
if mask.dtype() != DType::Bool {
return Err(MohuError::DomainError {
op: "index_bool",
reason: "mask dtype must be Bool".into(),
});
}
if mask.shape() != src.shape() {
return Err(MohuError::BoolIndexShapeMismatch {
index_shape: mask.shape().to_vec(),
array_shape: src.shape().to_vec(),
});
}
Comment thread
rajesh-puripanda marked this conversation as resolved.

let mut true_coords: Vec<Vec<usize>> = Vec::new();

for coord in NdIndexIter::new(src.shape()) {
if mask.get::<bool>(&coord)? {
true_coords.push(coord.to_vec());
}
}

let out_len = true_coords.len();
let mut out = Buffer::zeros(src.dtype(), &[out_len])?;

if out_len == 0 {
return Ok(out);
}

macro_rules! copy_bool {
($T:ty) => {
for (i, coord) in true_coords.iter().enumerate() {
let val = src.get::<$T>(coord)?;
out.set::<$T>(&[i], val)?;
}
Ok::<_, MohuError>(())
};
}

dispatch_dtype!(src.dtype(), copy_bool)?;

Ok(out)
}

#[cfg(test)]
mod tests {
use super::*;
use mohu_buffer::buffer::Buffer;

#[test]
fn test_index_bool_1d() {
let src = Buffer::from_slice::<i64>(&[10, 20, 30, 40, 50]).unwrap();
let mask = Buffer::from_slice::<bool>(&[true, false, true, false, true]).unwrap();
let result = index_bool(&src, &mask).unwrap();
let expected = Buffer::from_slice::<i64>(&[10, 30, 50]).unwrap();
assert_eq!(result.as_slice::<i64>().unwrap(), expected.as_slice::<i64>().unwrap());
}

#[test]
fn test_index_bool_all_false() {
let src = Buffer::from_slice::<i64>(&[1, 2, 3]).unwrap();
let mask = Buffer::from_slice::<bool>(&[false, false, false]).unwrap();
let result = index_bool(&src, &mask).unwrap();
assert_eq!(result.len(), 0);
}

#[test]
fn test_index_bool_all_true() {
let src = Buffer::from_slice::<f64>(&[1.0, 2.0, 3.0]).unwrap();
let mask = Buffer::from_slice::<bool>(&[true, true, true]).unwrap();
let result = index_bool(&src, &mask).unwrap();
assert_eq!(result.as_slice::<f64>().unwrap(), &[1.0, 2.0, 3.0]);
}
Comment thread
rajesh-puripanda marked this conversation as resolved.

#[test]
fn test_index_bool_2d() {
let data = &[&[1i64, 2, 3], &[4, 5, 6]];
let src = Buffer::from_slice_2d::<i64>(data).unwrap();
let mask = Buffer::from_slice_2d::<bool>(&[&[true, false, true], &[false, true, false]]).unwrap();
let result = index_bool(&src, &mask).unwrap();
let expected = Buffer::from_slice::<i64>(&[1, 3, 5]).unwrap();
assert_eq!(result.as_slice::<i64>().unwrap(), expected.as_slice::<i64>().unwrap());
}

#[test]
fn test_index_bool_wrong_dtype() {
let src = Buffer::from_slice::<i64>(&[1, 2, 3]).unwrap();
let mask = Buffer::from_slice::<i64>(&[1, 0, 1]).unwrap();
assert!(index_bool(&src, &mask).is_err());
}

#[test]
fn test_index_bool_shape_mismatch() {
let src = Buffer::from_slice::<i64>(&[1, 2, 3]).unwrap();
let mask = Buffer::from_slice::<bool>(&[true, false]).unwrap();
assert!(index_bool(&src, &mask).is_err());
}
}
210 changes: 209 additions & 1 deletion crates/mohu-index/src/take.rs
Original file line number Diff line number Diff line change
@@ -1 +1,209 @@
// take — implementation pending
use mohu_buffer::buffer::Buffer;
use mohu_buffer::layout::Order;
use mohu_buffer::strides::NdIndexIter;
use mohu_dtype::dispatch_dtype;
use mohu_dtype::dtype::DType;
use mohu_error::{MohuError, MohuResult};

/// Take elements from a buffer along an axis using an array of indices.
///
/// Equivalent to NumPy's `take(a, indices, axis)`.
///
/// # Errors
///
/// - `AxisOutOfRange` if `axis >= src.ndim()`
/// - `DomainError` if indices is not a 1D I64 buffer
/// - `FancyIndexOutOfBounds` if any index is out of range for the axis
pub fn index_take(src: &Buffer, indices: &Buffer, axis: usize) -> MohuResult<Buffer> {
let ndim = src.ndim();
if ndim == 0 {
return Err(MohuError::TooManyIndices {
given: 1,
ndim: 0,
});
}
if axis >= ndim {
return Err(MohuError::AxisOutOfRange {
axis: axis as i64,
ndim,
});
}
if indices.ndim() != 1 || indices.dtype() != DType::I64 {
return Err(MohuError::DomainError {
op: "index_take",
reason: "indices must be a 1D I64 array".into(),
});
}
Comment on lines +17 to +36

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
ast-grep --pattern 'pub enum MohuError { $$$ }'
rg -nP '\b(FancyIndexOutOfBounds|TooManyIndices|AxisOutOfRange|IndexOutOfBounds)\b' --type=rust -g '*error*' -C1

Repository: mohu-org/mohu

Length of output: 37948


Fix index_take error construction + use ensure!/bail! for guards

  • In crates/mohu-index/src/take.rs, MohuError::AxisOutOfRange must include the required valid: String field (the current AxisOutOfRange { axis: ..., ndim: ... } construction omits it).
  • Replace the guard/out-of-bounds return Err(MohuError::...) paths with ensure!/bail! per the project error-handling rules.
  • Double-check the empty-input/output shape handling and update any float equality assertions in tests to use mohu_testing::approx::assert_allclose.
🤖 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-index/src/take.rs` around lines 17 - 36, The error handling in
index_take needs to use the project's ensure!/bail! macros and construct
AxisOutOfRange with its required valid string: replace the explicit returns of
Err(MohuError::TooManyIndices { .. }), Err(MohuError::AxisOutOfRange { .. }),
and Err(MohuError::DomainError { .. }) in the function index_take with ensure!
or bail! calls (e.g., ensure!(ndim != 0, TooManyIndices{...}) or
bail!(DomainError{...}) as appropriate), and when constructing
MohuError::AxisOutOfRange include the missing valid: String field (for example
"0..ndim") so the variant is fully initialized; also verify the
empty-input/output shape handling remains correct and update any tests that
compare floats to use mohu_testing::approx::assert_allclose instead of exact
equality.


let axis_size = src.shape()[axis];
let indices_slice = indices.as_slice::<i64>()?;
let num_indices = indices_slice.len();

let mut wrapped_indices: Vec<usize> = Vec::with_capacity(num_indices);
for &idx in indices_slice.iter() {
let wrapped = if idx < 0 {
let w = idx + axis_size as i64;
if w < 0 {
return Err(MohuError::FancyIndexOutOfBounds {
index: idx,
axis,
size: axis_size,
});
}
w as usize
} else {
idx as usize
};
if wrapped >= axis_size {
return Err(MohuError::FancyIndexOutOfBounds {
index: idx,
axis,
size: axis_size,
});
}
wrapped_indices.push(wrapped);
}

if num_indices == 0 || src.len() == 0 {
let mut out_shape: Vec<usize> = src.shape().to_vec();
out_shape[axis] = 0;
return Buffer::zeros(src.dtype(), &out_shape);
}
Comment on lines +67 to +71

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Empty-source path produces wrong output shape along the axis.

When src.len() == 0 because a non-axis dimension is zero (e.g. shape [0, 3], axis = 1, indices = [0, 1]), the bounds checks pass but this branch forces out_shape[axis] = 0, yielding [0, 0] instead of the expected [0, num_indices] (NumPy take semantics). Using num_indices also stays correct for the num_indices == 0 case.

🐛 Proposed fix
     if num_indices == 0 || src.len() == 0 {
         let mut out_shape: Vec<usize> = src.shape().to_vec();
-        out_shape[axis] = 0;
+        out_shape[axis] = num_indices;
         return Buffer::zeros(src.dtype(), &out_shape);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if num_indices == 0 || src.len() == 0 {
let mut out_shape: Vec<usize> = src.shape().to_vec();
out_shape[axis] = 0;
return Buffer::zeros(src.dtype(), &out_shape);
}
if num_indices == 0 || src.len() == 0 {
let mut out_shape: Vec<usize> = src.shape().to_vec();
out_shape[axis] = num_indices;
return Buffer::zeros(src.dtype(), &out_shape);
}
🤖 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-index/src/take.rs` around lines 67 - 71, The branch that handles
empty-source currently sets out_shape[axis] = 0 which is incorrect when the
source is empty due to a non-axis dimension; instead set out_shape[axis] =
num_indices so the output shape matches NumPy take semantics (works for both
src.len() == 0 and num_indices == 0). Update the code around the if checking
num_indices == 0 || src.len() == 0 in take.rs to assign out_shape[axis] =
num_indices before returning Buffer::zeros(src.dtype(), &out_shape).


// Build output shape
let mut out_shape: Vec<usize> = src.shape().to_vec();
out_shape[axis] = num_indices;
let mut out = Buffer::alloc(src.dtype(), &out_shape, Order::C)?;

// Build reduced shape (all dims except axis)
let mut reduced_shape: Vec<usize> = Vec::with_capacity(ndim - 1);
for i in 0..ndim {
if i != axis {
reduced_shape.push(src.shape()[i]);
}
}

let mut src_coord = vec![0usize; ndim];
let mut out_coord = vec![0usize; ndim];

macro_rules! copy_take {
($T:ty) => {
if ndim == 1 {
for (pos, &wrapped) in wrapped_indices.iter().enumerate() {
let val = src.get::<$T>(&[wrapped])?;
out.set::<$T>(&[pos], val)?;
}
} else {
for reduced_coord in NdIndexIter::new(&reduced_shape) {
let mut ri = 0;
for i in 0..ndim {
if i != axis {
src_coord[i] = reduced_coord[ri];
out_coord[i] = reduced_coord[ri];
ri += 1;
}
}

for (pos, &wrapped) in wrapped_indices.iter().enumerate() {
src_coord[axis] = wrapped;
out_coord[axis] = pos;
let val = src.get::<$T>(&src_coord)?;
out.set::<$T>(&out_coord, val)?;
}
}
}
Ok::<_, MohuError>(())
};
}

dispatch_dtype!(src.dtype(), copy_take)?;

Ok(out)
}

#[cfg(test)]
mod tests {
use super::*;
use mohu_buffer::buffer::Buffer;

#[test]
fn test_index_take_1d() {
let src = Buffer::from_slice::<i64>(&[10, 20, 30, 40, 50]).unwrap();
let idx = Buffer::from_slice::<i64>(&[0, 2, 4]).unwrap();
let result = index_take(&src, &idx, 0).unwrap();
let expected = Buffer::from_slice::<i64>(&[10, 30, 50]).unwrap();
assert_eq!(result.as_slice::<i64>().unwrap(), expected.as_slice::<i64>().unwrap());
}

#[test]
fn test_index_take_1d_axis0() {
let src = Buffer::from_slice::<f64>(&[1.0, 2.0, 3.0, 4.0]).unwrap();
let idx = Buffer::from_slice::<i64>(&[3, 1]).unwrap();
let result = index_take(&src, &idx, 0).unwrap();
assert_eq!(result.as_slice::<f64>().unwrap(), &[4.0, 2.0]);
}

#[test]
fn test_index_take_2d_axis0() {
let data = &[&[1i64, 2], &[3, 4], &[5, 6]];
let src = Buffer::from_slice_2d::<i64>(data).unwrap();
let idx = Buffer::from_slice::<i64>(&[0, 2]).unwrap();
let result = index_take(&src, &idx, 0).unwrap();
assert_eq!(result.shape(), &[2, 2]);
assert_eq!(result.as_slice::<i64>().unwrap(), &[1, 2, 5, 6]);
}

#[test]
fn test_index_take_2d_axis1() {
let data = &[&[1i64, 2, 3], &[4, 5, 6]];
let src = Buffer::from_slice_2d::<i64>(data).unwrap();
let idx = Buffer::from_slice::<i64>(&[2, 0]).unwrap();
let result = index_take(&src, &idx, 1).unwrap();
assert_eq!(result.shape(), &[2, 2]);
assert_eq!(result.as_slice::<i64>().unwrap(), &[3, 1, 6, 4]);
}

#[test]
fn test_index_take_negative_indices() {
let src = Buffer::from_slice::<i64>(&[10, 20, 30]).unwrap();
let idx = Buffer::from_slice::<i64>(&[-1, -2]).unwrap();
let result = index_take(&src, &idx, 0).unwrap();
assert_eq!(result.as_slice::<i64>().unwrap(), &[30, 20]);
}

#[test]
fn test_index_take_empty_indices() {
let src = Buffer::from_slice::<i64>(&[1, 2, 3]).unwrap();
let idx = Buffer::from_slice::<i64>(&[]).unwrap();
let result = index_take(&src, &idx, 0).unwrap();
assert_eq!(result.len(), 0);
}

#[test]
fn test_index_take_out_of_bounds() {
let src = Buffer::from_slice::<i64>(&[1, 2, 3]).unwrap();
let idx = Buffer::from_slice::<i64>(&[5]).unwrap();
assert!(index_take(&src, &idx, 0).is_err());
}

#[test]
fn test_index_take_negative_out_of_bounds() {
let src = Buffer::from_slice::<i64>(&[1, 2, 3]).unwrap();
let idx = Buffer::from_slice::<i64>(&[-5]).unwrap();
assert!(index_take(&src, &idx, 0).is_err());
}

#[test]
fn test_index_take_wrong_dtype() {
let src = Buffer::from_slice::<i64>(&[1, 2, 3]).unwrap();
let idx = Buffer::from_slice::<f64>(&[1.0]).unwrap();
assert!(index_take(&src, &idx, 0).is_err());
}

#[test]
fn test_index_take_bad_axis() {
let src = Buffer::from_slice::<i64>(&[1, 2, 3]).unwrap();
let idx = Buffer::from_slice::<i64>(&[0]).unwrap();
assert!(index_take(&src, &idx, 1).is_err());
}
}
Loading
Loading