Add point preceding filter#379
Closed
tomjholland wants to merge 18 commits intomainfrom
Closed
Conversation
Replace procedural filtering functions with a new Filter class that encapsulates group-filtering logic, and mixin classes (CycleFiltersMixin, StepFiltersMixin) for cleaner separation of concerns. This refactoring improves code organization and reusability while maintaining existing API compatibility through method inheritance. Key changes: - Add Filter class with _build_mask(), _expand_positions(), etc. for handling complex index patterns (int, range, slice) with support for negative indexing - Add helper functions for mask manipulation and group counting - Introduce CycleFiltersMixin and StepFiltersMixin to provide cycle/step filtering methods to appropriate classes - Modify Procedure, Experiment, Cycle, Step to inherit from mixins - Add include_preceding_point parameter to experiment() method
Extract _make_constant_condition() helper to consolidate CC/CV filtering logic. Add target and rtol parameters to constant_voltage/constant_current methods: - target: explicitly specify the constant value to match (e.g., target=1.0 for 1A). Sign is preserved: target=-1.0 only matches negative values. - rtol: relative tolerance controlling the acceptance band as a fraction of |target|. Defaults to 0.001 (0.1%). When target is None, methods use the global mode of the column (backward-compatible behaviour). Includes comprehensive test coverage for all parameter combinations.
- cover target-based constant current and voltage assertions - add slice and include_preceding_point behavior tests - add iterator coverage and nested cycle chaining checks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #379 +/- ##
==========================================
+ Coverage 95.42% 95.87% +0.45%
==========================================
Files 58 58
Lines 5003 5485 +482
==========================================
+ Hits 4774 5259 +485
+ Misses 229 226 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Collaborator
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernises PyProBE’s filtering system by consolidating group-filtering logic into a private _Filter helper, adding iterator equivalents for filters, introducing include_preceding_point across filtering APIs, and extending constant-current/constant-voltage filters to support explicit target and rtol selection. It also fixes cycling summary ordering and updates tests/docs/contributor tooling accordingly.
Changes:
- Refactor filtering internals around
_Filter, additer_*filter variants, and addinclude_preceding_pointsupport across Procedure/Experiment/Cycle/Step filters. - Add parametric constant-value filtering via
targetandrtolforconstant_current()/constant_voltage(), with updated docs and tests. - Ensure cycling summary results are returned in a consistent order by sorting on
Cycle.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pyprobe/filters.py |
Core refactor introducing _Filter, slice/range handling, iterator filters, include_preceding_point, and parametric constant filters. |
tests/test_filter.py |
Major expansion/restructure of filter tests (unit + integration), including iterators, slicing, constant target/rtol, and preceding point behaviour. |
tests/analysis/test_degradation_mode_analysis.py |
Updates analysis test expectations to use the new constant-current target-based filter call. |
pyprobe/analysis/cycling.py |
Sorts cycling summary output by Cycle for stable ordering. |
docs/source/examples/filtering-data.ipynb |
Updates filtering examples for target usage, adds include_preceding_point, and demonstrates iterator filters. |
CONTRIBUTING.md |
Refreshes contributor guidance (uv-based setup, standards, and commands). |
.pre-commit-config.yaml |
Adjusts Ruff hook configuration. |
.gitignore |
Updates ignore patterns (including coding-agent artefacts) and attempts to special-case GitHub workflows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+205
to
+210
| step_val = idx.step if idx.step is not None else 1 | ||
| start = idx.start if idx.start is not None else 0 | ||
| if start >= 0 and idx.stop is not None and idx.stop >= 0: | ||
| positions.extend(range(start, idx.stop, step_val)) | ||
| else: | ||
| positions.extend(range(*idx.indices(get_total()))) |
Comment on lines
+460
to
+464
| if target is not None: | ||
| return (pl.col(col) - target).abs() <= abs(target) * rtol | ||
| mode_expr = pl.col(col).filter(mask) if mask is not None else pl.col(col) | ||
| t = mode_expr.mode().first() | ||
| return (pl.col(col) - t).abs() <= t.abs() * rtol |
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.
Summary
Refactors filter architecture using a private Filter class with mixin-based filtering methods, adds parametric constant-value filtering with
targetandrtolparameters, introducesinclude_preceding_pointoption across all filter methods, and fixes cycling summary result ordering. Closes #374, #363Features
targetandrtolparameters toconstant_voltage()andconstant_current()for explicit constant-value matching with configurable tolerance (946623e1)include_preceding_pointparameter to all filter methods across Procedure, Experiment, Cycle, and Step to optionally include the row before each selected group (ae3abcfc)Refactoring
_Filterclass encapsulating group-filtering logic with_build_mask(),_expand_positions(), and helper utilities (ae3abcfc)Filterclass private with final API cleanup (81a47237)Fixes
Tests
include_preceding_pointassertions, iterator coverage, and nested cycle chaining (29af1578)Docs
include_preceding_pointfeatures (1033f7d4)Chores