Conversation
* Fix review: update doc.md/quickstart naming, simplify top_k slicing Addresses PR #494 review comments: - doc.md: classify_audios_in_windows → classify_audios(..., win_length=) - doc.md: window_size/hop_size → win_length/hop_length - quickstart.md: same naming fix - Simplified top_k: direct list slicing (handles empty/short lists) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix: apply top_k slicing consistently to final batch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request renames the windowed classification function to classify_audios and updates its parameter names to win_length and hop_length for consistency. Documentation and examples have been updated to reflect these changes. Review feedback indicates that the top_k parameter is not being passed to the underlying classification call, which may limit the number of labels returned regardless of the user's input. Additionally, there is a suggestion to refactor duplicated logic used for processing and storing classification results to improve maintainability.
| "labels": result.labels[:top_k], | ||
| "scores": result.scores[:top_k], |
There was a problem hiding this comment.
The top_k parameter is used here for slicing the results, but it is not passed to the _classify_whole call on line 131 (and line 148). This means the underlying classifier will use its default top_k value (typically 5). If a user requests a top_k value larger than the model's default, they will still only receive the default number of labels. Consider passing top_k=top_k to _classify_whole to ensure the requested number of labels is retrieved from the model. Additionally, the default value of 5 for windowed mode (set in classify_audios) contradicts the docstring which states that None should keep all labels.
| audio_results.append( | ||
| { | ||
| "start": bp / sr, | ||
| "end": min(bp + win_samples, n_samples) / sr, | ||
| "labels": result.labels[:k], | ||
| "scores": result.scores[:k], | ||
| "labels": result.labels[:top_k], | ||
| "scores": result.scores[:top_k], | ||
| "win_length": win_length, | ||
| "hop_length": hop_length, | ||
| } |
There was a problem hiding this comment.
The logic for processing classification results and appending them to audio_results is duplicated here and on lines 133-141. Consider refactoring this into a small helper function or restructuring the loop to avoid code duplication and improve maintainability.
1 participant
Fix doc.md/quickstart naming, consistent top_k slicing.