Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional serde serialization support for configuration types in soundevents, along with some API adjustments to make the option builders more consistent and less move-heavy.
Changes:
- Add
serde(feature-gated) derives and defaults forOptions,ChunkingOptions, andChunkAggregation. - Refactor
Options::new/ChunkingOptionsconstructors and builder methods; introduceset_*mutators forChunkingOptions. - Change chunked classification APIs to take
&ChunkingOptions(and update in-crate call sites/tests accordingly), and bump crate version to0.3.0.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| soundevents/src/lib.rs | Adds serde derives/defaults and adjusts option constructors/builders; changes chunked APIs to borrow options. |
| soundevents/Cargo.toml | Bumps soundevents version to 0.3.0; adds serde feature + optional dependency and dev-dependency serde_json. |
| README.md | Updates dependency snippet to soundevents = "0.3". |
| CHANGELOG.md | Adds 0.3.0 release entry mentioning serde support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn classify_chunked( | ||
| &mut self, | ||
| samples_16k: &[f32], | ||
| top_k: usize, | ||
| options: ChunkingOptions, | ||
| options: &ChunkingOptions, | ||
| ) -> Result<Vec<EventPrediction>, ClassifierError> { |
There was a problem hiding this comment.
classify_all_chunked/classify_chunked now take &ChunkingOptions, which makes the README/rustdoc examples that pass opts by value fail to compile (e.g. the chunked inference example in soundevents/README.md that’s include_str!’d into crate docs). Update those examples to pass &opts (or adjust the API to accept impl Borrow<ChunkingOptions> / by-value if you want to keep the old call style).
| /// Options for chunked inference over long clips. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| pub struct ChunkingOptions { | ||
| #[cfg_attr(feature = "serde", serde(default = "default_window_samples"))] | ||
| window_samples: usize, | ||
| #[cfg_attr(feature = "serde", serde(default = "default_hop_samples"))] | ||
| hop_samples: usize, | ||
| #[cfg_attr(feature = "serde", serde(default = "default_batch_size"))] | ||
| batch_size: usize, | ||
| #[cfg_attr(feature = "serde", serde(default))] | ||
| aggregation: ChunkAggregation, | ||
| } |
There was a problem hiding this comment.
Serde support was added for ChunkingOptions/ChunkAggregation, but there are no tests exercising their serialization/deserialization (including defaulting behavior and snake_case renames). Add #[cfg(feature = "serde")] tests similar to test_serde for Options to ensure {} deserializes to defaults and that aggregation round-trips as expected.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
No description provided.