feat(io): add CSV I/O (Fixes #228) - #237
Conversation
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com> (cherry picked from commit 7bbd6db) Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
…ssary casts) Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
…and fft modules Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces a typed CSV reader/writer in mohu-io, implements core FFT utilities in mohu-fft, and includes a few small cleanups across supporting crates.
Changes:
- Added
mohu-io::csvmodule with typed inference, configurable read/write options, and convenience re-exports. - Implemented 1-D
fft/ifftplusfftfreq/fftshifthelpers inmohu-fft, with basic unit tests. - Minor formatting/metadata tweaks in
mohu-error,mohu-dtype, and workspace dependencies.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/mohu-io/src/csv.rs | Adds CSV reader/writer implementation and public API. |
| crates/mohu-io/src/lib.rs | Re-exports CSV API from crate root. |
| crates/mohu-io/tests/csv_tests.rs | Adds unit tests for CSV reader/writer behavior. |
| crates/mohu-io/Cargo.toml | Adds csv workspace dependency. |
| crates/mohu-fft/src/transform.rs | Implements fft/ifft wrappers around rustfft with normalization. |
| crates/mohu-fft/src/freq.rs | Implements fftfreq/rfftfreq/fftshift utilities with tests. |
| crates/mohu-fft/Cargo.toml | Adds cargo-machete ignored dependency metadata. |
| crates/mohu-error/src/test_utils.rs | Reorders imports and compacts panic formatting. |
| crates/mohu-error/src/reporter.rs | Formatting/alignment adjustments in formatting code paths. |
| crates/mohu-dtype/src/macros.rs | Removes an extra blank doc line. |
| crates/mohu-dtype/src/finfo.rs | Simplifies numeric casts in precision computations. |
| crates/mohu-dtype/src/dtype.rs | Adds FromStr impl for DType. |
| Cargo.toml | Adds workspace dependency on csv. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Failure reported by the `csv` crate. | ||
| #[error("CSV parse error: {0}")] | ||
| Parse(#[from] csv::Error), |
| let mut csv_reader = csv::ReaderBuilder::new() | ||
| .delimiter(self.opts.delimiter) | ||
| .has_headers(false) | ||
| .comment(self.opts.comment) | ||
| .trim(csv::Trim::All) | ||
| .flexible(true) | ||
| .from_reader(reader); |
| let mut csv_writer = csv::WriterBuilder::new() | ||
| .delimiter(self.opts.delimiter) | ||
| .has_headers(false) | ||
| .terminator(csv::Terminator::Any(b'\n')) | ||
| .from_writer(&mut row_bytes); |
| impl std::str::FromStr for DType { | ||
| type Err = MohuError; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| DType::from_str(s) | ||
| } | ||
| } |
| pub use csv::{ | ||
| read_csv, write_csv, CsvError, CsvReader, CsvResult, CsvTable, CsvValue, CsvWriter, | ||
| ReadOptions, WriteOptions, | ||
| }; |
| fn write_impl<W: Write>(&self, table: &CsvTable, mut writer: W) -> CsvResult<()> { | ||
| if self.opts.write_header && !table.headers.is_empty() { | ||
| self.write_row(&mut writer, table.headers.iter().map(String::as_str))?; | ||
| } | ||
|
|
||
| for row in &table.data { | ||
| let values = row.iter().map(|value| { | ||
| if matches!(value, CsvValue::Missing) { | ||
| self.opts.missing_repr.as_str().to_owned() | ||
| } else { | ||
| value.to_csv_string() | ||
| } | ||
| }).collect::<Vec<_>>(); | ||
|
|
||
| self.write_row(&mut writer, values.iter().map(String::as_str))?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn write_row<W, I, S>(&self, writer: &mut W, fields: I) -> CsvResult<()> | ||
| where | ||
| W: Write, | ||
| I: IntoIterator<Item = S>, | ||
| S: AsRef<str>, | ||
| { | ||
| let mut row_bytes = Vec::new(); | ||
| { | ||
| let mut csv_writer = csv::WriterBuilder::new() | ||
| .delimiter(self.opts.delimiter) | ||
| .has_headers(false) | ||
| .terminator(csv::Terminator::Any(b'\n')) | ||
| .from_writer(&mut row_bytes); | ||
|
|
||
| csv_writer.write_record(fields)?; | ||
| csv_writer.flush()?; | ||
| } |
| } | ||
|
|
||
| /// Shift the zero-frequency component to the center of the spectrum. | ||
| pub fn fftshift<T: Clone>(mut v: Vec<T>) -> Vec<T> { |
| } | ||
|
|
||
| /// The inverse of `fftshift`. | ||
| pub fn ifftshift<T: Clone>(mut v: Vec<T>) -> Vec<T> { |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a typed CSV reader/writer and tests (mohu-io), implements FFT frequency helpers and 1‑D FFT/IFFT (mohu-fft), introduces DType FromStr parsing, and applies broad formatting/reorganization and small API-adjacent edits across many crates plus Windows helper docs/script. ChangesCSV I/O Module
FFT Transform and Frequency Utilities
Type System and Error Handling Polish
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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-dtype/src/dtype.rs`:
- Around line 526-532: Remove the inherent parsing method DType::from_str (the
pub fn from_str on DType) and update all call sites to use the FromStr trait
implementation instead; specifically change any direct calls to
DType::from_str(s) in the TryFrom<&str> impls and the serde Deserialize code to
either s.parse::<DType>() or <DType as std::str::FromStr>::from_str(s) so the
trait implementation is used and error types (MohuError) are preserved.
In `@crates/mohu-fft/src/freq.rs`:
- Around line 23-24: Update the docstring for rfftfreq to accurately describe
its behavior: state that unlike fftfreq, rfftfreq returns only the non-negative
frequency bins (length n/2 + 1 for even n, floor(n/2) + 1 generally) for
real-input transforms, and clarify that it does not return the
negative-frequency counterparts; reference the function name rfftfreq and
fftfreq so readers know which behavior differs.
- Line 66: Replace the exact equality check on the float vector with
tolerance-based comparison: change the assertion that uses assert_eq!(f,
vec![0.0, 0.25, -0.5, -0.25]) to use moh_testing::approx::assert_allclose! so
the Vec<f64> in variable f is compared with the expected vec using approximate
(tolerance) comparison; keep the same expected values and use the macro path
moh_testing::approx::assert_allclose! to avoid adding new imports.
In `@crates/mohu-io/src/csv.rs`:
- Around line 156-162: The column() method and write_impl() in CsvTable assume
every row has length ncols and directly index row[idx], which can panic or
produce ragged output for non-rectangular tables; update column(), write_impl(),
and any public constructors/mutators that can create CsvTable.data so they
validate row widths or guard accesses: in column() iterate rows and use get(idx)
(or skip/return None for rows too short) and return Option<Vec<&CsvValue>> only
when all rows contain that index; in write_impl() validate each row length
against ncols (either pad, skip, or error) before emitting and return a Result
to propagate width errors; ensure CsvTable invariants are restored/checked on
construction and mutation so data and ncols remain consistent.
- Around line 13-47: Replace the local CsvError/CsvResult types and all manual
Err returns in this module with the project-wide MohuError from mohu-error:
remove the CsvError enum and CsvResult alias, change function return types to
Result<..., MohuError>, and convert places that currently propagate errors or
use `return Err(...)` to use `?` with `.context("...")` or `.with_context(||
"...")` to add operation-specific context; replace validation-style `return
Err(...)` with `ensure!(condition, "message with {}", arg)` and use
`bail!("message with {}", arg)` for early error returns so code uses
MohuError-compatible macros (bail!/ensure!) and context wrapping throughout
(including the other referenced blocks).
In `@crates/mohu-io/src/lib.rs`:
- Around line 6-9: Add a root-level doc comment above the pub use re-export
block that documents the CSV public API (read_csv, write_csv, CsvError,
CsvReader, CsvResult, CsvTable, CsvValue, CsvWriter, ReadOptions, WriteOptions)
and include at least one `# Example` code block demonstrating basic usage (e.g.,
reading or writing a CSV using read_csv/write_csv). Ensure the doc comment is
placed immediately above the existing `pub use csv::{ ... }` line and describes
the purpose of the re-exports and a minimal, compile-ready example showing how a
consumer would call one of the re-exported functions or types.
In `@crates/mohu-io/tests/csv_tests.rs`:
- Around line 39-40: The test uses exact equality on a float value
(assert_eq!(table.data[0][2], CsvValue::Float(9.5))) which violates the
float-comparison guideline; replace this with a call to
moh_testing::approx::assert_allclose by extracting the f64 from table.data[0][2]
(matching on CsvValue::Float) and passing the actual f64 and the expected 9.5 to
assert_allclose (optionally with a small tolerance), ensuring you import
moh_testing::approx::assert_allclose or fully qualify the call.
🪄 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: 7770972f-287b-43ac-8c59-d75010676f02
📒 Files selected for processing (13)
Cargo.tomlcrates/mohu-dtype/src/dtype.rscrates/mohu-dtype/src/finfo.rscrates/mohu-dtype/src/macros.rscrates/mohu-error/src/reporter.rscrates/mohu-error/src/test_utils.rscrates/mohu-fft/Cargo.tomlcrates/mohu-fft/src/freq.rscrates/mohu-fft/src/transform.rscrates/mohu-io/Cargo.tomlcrates/mohu-io/src/csv.rscrates/mohu-io/src/lib.rscrates/mohu-io/tests/csv_tests.rs
💤 Files with no reviewable changes (1)
- crates/mohu-dtype/src/macros.rs
| impl std::str::FromStr for DType { | ||
| type Err = MohuError; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| DType::from_str(s) | ||
| } | ||
| } |
There was a problem hiding this comment.
CI-blocking clippy error: inherent from_str method conflicts with FromStr trait.
The pipeline shows a clippy::should_implement_trait error at line 463 because both an inherent method DType::from_str and this FromStr trait implementation exist. CI runs with -D warnings, so this blocks the build.
The FromStr trait is the idiomatic Rust approach for parsing from strings. Remove the inherent from_str method at line 463 and update all call sites (lines 536-544 TryFrom impls, line 559 serde deserialize) to use the trait via .parse() or qualify as <DType as FromStr>::from_str(s).
🔧 Proposed fix
Step 1: Delete the inherent method at lines 463-510 (the entire pub fn from_str block).
Step 2: Update call sites to use the trait. For example, the TryFrom<&str> impl at line 538 becomes:
impl TryFrom<&str> for DType {
type Error = MohuError;
- fn try_from(s: &str) -> MohuResult<Self> { DType::from_str(s) }
+ fn try_from(s: &str) -> MohuResult<Self> { s.parse() }
}Similarly, update line 543 and line 559 (serde).
As per coding guidelines: "MSRV: 1.85 in clippy.toml — CI runs -D warnings for zero warnings allowed. Fix clippy, do not #[allow] unless the lint is genuinely wrong, and document why."
🤖 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-dtype/src/dtype.rs` around lines 526 - 532, Remove the inherent
parsing method DType::from_str (the pub fn from_str on DType) and update all
call sites to use the FromStr trait implementation instead; specifically change
any direct calls to DType::from_str(s) in the TryFrom<&str> impls and the serde
Deserialize code to either s.parse::<DType>() or <DType as
std::str::FromStr>::from_str(s) so the trait implementation is used and error
types (MohuError) are preserved.
| use thiserror::Error; | ||
|
|
||
| /// Errors produced while reading or writing CSV data. | ||
| #[derive(Debug, Error)] | ||
| pub enum CsvError { | ||
| /// Underlying I/O failure. | ||
| #[error("I/O error: {0}")] | ||
| Io(#[from] std::io::Error), | ||
|
|
||
| /// Failure reported by the `csv` crate. | ||
| #[error("CSV parse error: {0}")] | ||
| Parse(#[from] csv::Error), | ||
|
|
||
| /// UTF-8 conversion failed while producing a `String` output. | ||
| #[error("UTF-8 conversion error: {0}")] | ||
| Utf8(#[from] std::string::FromUtf8Error), | ||
|
|
||
| /// No header or data rows were found. | ||
| #[error("empty file: no records found")] | ||
| EmptyFile, | ||
|
|
||
| /// A data row had a different number of columns than expected. | ||
| #[error("row {row} has {got} columns, expected {expected}")] | ||
| ColumnMismatch { | ||
| /// 1-based row index within the data section. | ||
| row: usize, | ||
| /// Expected number of columns. | ||
| expected: usize, | ||
| /// Actual number of columns. | ||
| got: usize, | ||
| }, | ||
| } | ||
|
|
||
| /// Result type used by CSV reader and writer operations. | ||
| pub type CsvResult<T> = Result<T, CsvError>; |
There was a problem hiding this comment.
Align this module with the repository error-handling contract.
This code introduces CsvError/CsvResult, uses manual return Err(...), and propagates fallible ops without required context. That breaks the project-wide error model and will make cross-crate error reporting inconsistent.
As per coding guidelines, "Use MohuError from mohu-error everywhere for error handling", "Wrap error context with .context(\"what was being attempted\") or .with_context(|| ...)", and "Use bail! and ensure! macros from mohu-error — not manual return Err(...)."
Also applies to: 194-201, 223-226, 263-267, 338-347
🤖 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-io/src/csv.rs` around lines 13 - 47, Replace the local
CsvError/CsvResult types and all manual Err returns in this module with the
project-wide MohuError from mohu-error: remove the CsvError enum and CsvResult
alias, change function return types to Result<..., MohuError>, and convert
places that currently propagate errors or use `return Err(...)` to use `?` with
`.context("...")` or `.with_context(|| "...")` to add operation-specific
context; replace validation-style `return Err(...)` with `ensure!(condition,
"message with {}", arg)` and use `bail!("message with {}", arg)` for early error
returns so code uses MohuError-compatible macros (bail!/ensure!) and context
wrapping throughout (including the other referenced blocks).
| pub fn column(&self, idx: usize) -> Option<Vec<&CsvValue>> { | ||
| if idx >= self.ncols { | ||
| return None; | ||
| } | ||
|
|
||
| Some(self.data.iter().map(|row| &row[idx]).collect()) | ||
| } |
There was a problem hiding this comment.
Guard against non-rectangular CsvTable to prevent panics and ragged writes.
column() directly indexes rows (row[idx]) and write_impl() emits rows without width validation. Because CsvTable fields are public, inconsistent row widths can panic or silently produce malformed CSV.
Suggested hardening patch
pub fn column(&self, idx: usize) -> Option<Vec<&CsvValue>> {
if idx >= self.ncols {
return None;
}
-
- Some(self.data.iter().map(|row| &row[idx]).collect())
+ self.data.iter().map(|row| row.get(idx)).collect()
}- for row in &table.data {
+ for (row_index, row) in table.data.iter().enumerate() {
+ if row.len() != table.ncols {
+ return Err(CsvError::ColumnMismatch {
+ row: row_index + 1,
+ expected: table.ncols,
+ got: row.len(),
+ });
+ }
let values = row.iter().map(|value| {
if matches!(value, CsvValue::Missing) {
self.opts.missing_repr.as_str().to_owned()
} else {
value.to_csv_string()
}
}).collect::<Vec<_>>();Also applies to: 355-365
🤖 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-io/src/csv.rs` around lines 156 - 162, The column() method and
write_impl() in CsvTable assume every row has length ncols and directly index
row[idx], which can panic or produce ragged output for non-rectangular tables;
update column(), write_impl(), and any public constructors/mutators that can
create CsvTable.data so they validate row widths or guard accesses: in column()
iterate rows and use get(idx) (or skip/return None for rows too short) and
return Option<Vec<&CsvValue>> only when all rows contain that index; in
write_impl() validate each row length against ncols (either pad, skip, or error)
before emitting and return a Result to propagate width errors; ensure CsvTable
invariants are restored/checked on construction and mutation so data and ncols
remain consistent.
| pub use csv::{ | ||
| read_csv, write_csv, CsvError, CsvReader, CsvResult, CsvTable, CsvValue, CsvWriter, | ||
| ReadOptions, WriteOptions, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add root-level docs for the new CSV re-exports.
These newly exposed public API items in src/lib.rs should include a doc comment with at least one # Example block.
As per coding guidelines, "Public API items need doc comments with at least one # Example block."
🤖 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-io/src/lib.rs` around lines 6 - 9, Add a root-level doc comment
above the pub use re-export block that documents the CSV public API (read_csv,
write_csv, CsvError, CsvReader, CsvResult, CsvTable, CsvValue, CsvWriter,
ReadOptions, WriteOptions) and include at least one `# Example` code block
demonstrating basic usage (e.g., reading or writing a CSV using
read_csv/write_csv). Ensure the doc comment is placed immediately above the
existing `pub use csv::{ ... }` line and describes the purpose of the re-exports
and a minimal, compile-ready example showing how a consumer would call one of
the re-exported functions or types.
| assert_eq!(table.data[0][2], CsvValue::Float(9.5)); | ||
| } |
There was a problem hiding this comment.
Use approximate float comparison in this test.
This assertion currently relies on exact f64 equality through assert_eq!; switch it to mohu_testing::approx::assert_allclose.
As per coding guidelines, "Float comparisons in tests must use mohu_testing::approx::assert_allclose — never assert_eq! on f32 or f64."
🤖 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-io/tests/csv_tests.rs` around lines 39 - 40, The test uses exact
equality on a float value (assert_eq!(table.data[0][2], CsvValue::Float(9.5)))
which violates the float-comparison guideline; replace this with a call to
moh_testing::approx::assert_allclose by extracting the f64 from table.data[0][2]
(matching on CsvValue::Float) and passing the actual f64 and the expected 9.5 to
assert_allclose (optionally with a small tolerance), ensuring you import
moh_testing::approx::assert_allclose or fully qualify the call.
Signed-off-by: GitHub Copilot <noreply@github.com>
Signed-off-by: GitHub Copilot <noreply@github.com>
Signed-off-by: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/ci.yml:
- Line 1: The CI YAML was flattened into one line and contains a UTF-8 BOM and
garbled characters which breaks parsing; open the workflow text (look for the
top-level workflow name and jobs like dco, fmt, clippy, build) and restore it to
a proper multi-line YAML document by removing the BOM and any stray garbled
bytes/characters, reinserting newlines and indentation so keys (name:, on:,
env:, jobs:, etc.) and job blocks (dco -> steps -> - uses:) are valid YAML, and
ensure the DCO step uses christophebedard/dco-check@0.5.1 exactly; validate by
running a YAML linter or `act`/GitHub Actions YAML parser to confirm no "mapping
values" errors remain.
🪄 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: ea246b79-c519-4115-9b02-ddab37ac3160
📒 Files selected for processing (1)
.github/workflows/ci.yml
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Adds CSV read/write support to mohu-io with configurable delimiters, header handling, missing value inference, and memory-efficient parsing.\n\nFiles added/changed: crates/mohu-io/src/csv.rs, crates/mohu-io/src/lib.rs, crates/mohu-io/Cargo.toml, Cargo.toml, crates/mohu-io/tests/csv_tests.rs.\n\nNote: I attempted to run cargo test -p mohu-io locally but encountered Windows linker/toolchain issues (MSVC link.exe not found and GNU dlltool bfd mismatch). Please run CI to validate tests.\n\nFixes #228
Summary by CodeRabbit
New Features
Tests
Documentation