Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The missing-label error behavior conflicts with its documentation, and related documentation corrections remain needed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds BatchData.set_label() and label-based filter-set indexing.
Changes:
- Adds label assignment, lookup, validation, and lifecycle handling.
- Supports labels in filter-set operations.
- Documents label usage and combination behavior.
File summaries
| File | Summary | Review findings |
|---|---|---|
src/batch/interface.rs |
Implements filter-set labels and label resolution. | Update get_histogram and get_n_events docstrings to support int | str; fix the missing-label error type or remove the KeyError claim. |
docs/source/tutorials/batch_processing.ipynb |
Documents labeling and batch operations. | Clarify that + retains histogram settings while * creates fresh histograms. |
Review details
Suppressed comments (2)
docs/source/tutorials/batch_processing.ipynb:401
+does not reset histogram settings:add()clones the left operand'sresults, retaining its time range and bin count, whereas*constructs fresh histograms. This statement therefore misdocuments the behavior; distinguish the label loss from the different histogram-setting behavior of+and*.
"Note that as `+` and `*` create new filter sets from old ones, any existing histogram settings or labels are lost when using those operations."
src/batch/interface.rs:709
Error::msg(...)creates an anyhow message, not aPyErr, so PyO3'sanyhowconversion cannot downcast it toKeyError; a missing label will be exposed as aRuntimeError, contrary to this comment. Either propagate an actualPyKeyErrorthrough the error path or remove the claim about the exception type.
// PyO3 downcasts back out of anyhow, so this raises a real KeyError
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| self.check_index(*i)?; | ||
| Ok(vec![*i]) | ||
| } | ||
| FilterIndex::Label(label) => Ok(vec![self.label_to_index(label)?]), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds
BatchData.set_label()which allows you to set a label for a filter set and index the filter set using its label. Fixes #111.