Add display option for multi-scroll filter selection by column with threshold indicators#203
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughImplements per-column KDE-style filter sliders and rendering hooks in CytoDataFrame; updates tests, an example and README docs; bumps two pre-commit hooks and adds a per-file flake-ignore in pyproject.toml. Changes
Sequence DiagramsequenceDiagram
actor User
participant Widget as Filter Slider Widget
participant Frame as CytoDataFrame
participant Renderer as HTML/Trame Renderer
participant Display as Notebook Display
User->>Widget: adjust slider range
Widget->>Frame: _on_filter_slider_change(event)
Frame->>Frame: update _widget_state.filter_ranges
Frame->>Frame: compute filtered indices (_filter_display_indices_by_widget_range)
Frame->>Renderer: request render with filters
Renderer->>Renderer: build KDE SVG + slider controls
Renderer->>Display: emit filtered HTML (controls + rows)
Display->>User: show updated table
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/test_frame.py (2)
977-979: These exact SVG coordinate assertions are brittle.Line 977–Line 979 hard-code specific y-values (
6.00,22.00), so minor non-functional layout tuning can break tests. Prefer asserting semantic presence (threshold line exists with expected color) plus relative placement tolerances.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_frame.py` around lines 977 - 979, The three brittle assertions check exact SVG y coordinates in control.children[0].value; instead parse the SVG element string to assert the threshold line exists with stroke="#dc2626" and validate its y1/y2 numerically with a tolerance or relative check (e.g., parse y1 and y2 as floats and assert abs(y1-expected) < tol and abs(y2-expected) < tol or assert (y2 - y1) matches expected length within tolerance); update the assertions around control.children[0].value to use color presence plus tolerant/relative numeric comparisons rather than exact string equality.
933-933: Refactor to use the public CSS class API instead of the private_dom_classesattribute.The test asserts on
_dom_classes, which is an underscore-prefixed trait and not part of ipywidgets' stable public API. This creates potential version-coupling and brittle test failures. Use the public API methodsadd_class(...)/remove_class(...)to manage CSS classes, then verify the expected state through the widget's public contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_frame.py` at line 933, The test is inspecting the private attribute slider._dom_classes; change it to use ipywidgets' public class API: call slider.add_class("cdf-filter-range-slider") where the class is set (or ensure the code under test uses add_class) and then assert the presence via the public contract by checking slider.get_state()/widget serialization or another public accessor rather than _dom_classes — or assert that calling slider.remove_class("cdf-filter-range-slider") changes the widget state accordingly; reference the slider variable and replace any direct access to _dom_classes with add_class/remove_class and assertions that rely on public behavior (e.g., serialized state or class-manipulation effects).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cytodataframe/frame.py`:
- Around line 657-695: The slider is building one option per distinct numeric
value (unique_values → options) which can be thousands; instead cap the number
of stops before building the widget: introduce a constant (e.g.,
MAX_FILTER_SLIDER_STOPS) and if len(unique_values) > MAX_FILTER_SLIDER_STOPS,
replace options with a reduced set generated from the column domain (use min/max
from numeric_values and generate MAX_FILTER_SLIDER_STOPS evenly spaced tick
values or sample evenly from sorted unique_values), ensure
default_lower/default_upper and normalized_range use the capped domain bounds,
and keep the rest of the SelectionRangeSlider construction
(SelectionRangeSlider, options, value) unchanged so the widget remains
performant for near-unique numeric columns.
- Around line 4257-4287: display_indices is currently computed on the
pre-filtered head/tail window, causing matches in the full frame to be missed;
instead apply the widget filter(s) to the entire DataFrame first and only then
derive the displayed indices. Concretely: detect
active_filter_columns/active_filter_ranges from
self._custom_attrs["_widget_state"], apply those ranges to filter self (or local
data) across all rows (e.g. compute a mask for the tuple ranges and do data =
data.loc[mask] or use existing filtering helper), then call
self._filter_display_indices_by_widget_range(...) or compute display_indices =
data.index.tolist() for the head/tail window on the already-filtered data;
update the branches that currently do data = data.loc[display_indices] so they
operate on full-frame-filtered data instead. Ensure you keep the same symbol
names (display_indices, _filter_display_indices_by_widget_range,
active_filter_columns, active_filter_ranges, data) so the change is localized
and clear.
- Around line 4733-4769: _current implementation of
_try_render_trame_widget_table returns immediately after rendering the trame
widget snapshot which skips building sliders/threshold plots when
display_options contains filter_columns; update the function to detect when
display_options.get("filter_columns") is set (and non-empty) and in that case do
not short-circuit — either avoid the early return after display(...) /
display(HTML(...)) or explicitly call the code path that builds the notebook GUI
(e.g., invoke self._render_notebook_widget_output or the same helper used by it)
so sliders/threshold plots are created for frames that support 3D; touch the
_try_render_trame_widget_table function and the path around show_widget_table /
return True to ensure filters are respected.
In `@tests/test_frame.py`:
- Around line 982-1002: The test name is misleading: the test actually asserts
that an out-of-range threshold is rendered (red stroke) and a warning is logged,
so rename the test function to reflect that behavior (for example,
test_filter_slider_control_warns_and_clamps_out_of_range_threshold) and update
any references; locate the test function by its current name
test_filter_slider_control_ignores_out_of_range_threshold and the helper under
test _build_filter_slider_control_for_column, then update the function name and
its docstring or comment (if present) to match the expectation that the control
shows a clamped/out-of-range indicator and logs a warning.
---
Nitpick comments:
In `@tests/test_frame.py`:
- Around line 977-979: The three brittle assertions check exact SVG y
coordinates in control.children[0].value; instead parse the SVG element string
to assert the threshold line exists with stroke="#dc2626" and validate its y1/y2
numerically with a tolerance or relative check (e.g., parse y1 and y2 as floats
and assert abs(y1-expected) < tol and abs(y2-expected) < tol or assert (y2 - y1)
matches expected length within tolerance); update the assertions around
control.children[0].value to use color presence plus tolerant/relative numeric
comparisons rather than exact string equality.
- Line 933: The test is inspecting the private attribute slider._dom_classes;
change it to use ipywidgets' public class API: call
slider.add_class("cdf-filter-range-slider") where the class is set (or ensure
the code under test uses add_class) and then assert the presence via the public
contract by checking slider.get_state()/widget serialization or another public
accessor rather than _dom_classes — or assert that calling
slider.remove_class("cdf-filter-range-slider") changes the widget state
accordingly; reference the slider variable and replace any direct access to
_dom_classes with add_class/remove_class and assertions that rely on public
behavior (e.g., serialized state or class-manipulation effects).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 565d3134-c1a9-4f56-b579-84ef404a4182
📒 Files selected for processing (6)
.pre-commit-config.yamlREADME.mddocs/src/examples/cytodataframe_at_a_glance.ipynbdocs/src/examples/cytodataframe_at_a_glance.pysrc/cytodataframe/frame.pytests/test_frame.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_frame.py (2)
1159-1166: Consider extracting duplicated display-option patch setup.The option dictionaries and
get_optionmonkeypatch pattern are repeated. A small helper/fixture would reduce duplication and make future option-shape changes easier to maintain.Also applies to: 1191-1199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_frame.py` around lines 1159 - 1166, Extract the duplicated display-options dict and monkeypatch pattern into a single helper or pytest fixture (e.g., patch_display_options or display_options_fixture) and use that helper in tests instead of repeating the dict + monkeypatch.setattr calls; update the locations that currently call the inline options and monkeypatch.setattr (the blocks around the options variable and monkeypatch.setattr("cytodataframe.frame.get_option", ...)) to call the new helper/fixture so future changes to display options are made in one place.
1007-1009: Reduce brittleness from exact SVG coordinate/string assertions.A few checks depend on exact serialized formatting (e.g., fixed
y1/y2values and strict attribute patterns). Consider asserting semantic properties only (presence of threshold line + in-track position bounds) to keep tests stable across harmless SVG formatting/layout tweaks.Also applies to: 1089-1096
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_frame.py` around lines 1007 - 1009, Replace brittle exact-string assertions against control.children[0].value (e.g., "stroke='#dc2626'", "y1='6.00'", "y2='22.00'") with semantic checks: assert the SVG contains the threshold line (check for the stroke color attribute value rather than exact quoting/spacing) and extract the numeric y1/y2 values from the element string, then assert they lie within expected bounds or within a small tolerance range. Apply the same change to the similar assertions around lines 1089-1096 so tests validate presence and approximate position of the threshold line instead of exact serialized formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cytodataframe/frame.py`:
- Around line 4779-4785: The trame bypass guard only inspects
display_options.get("filter_columns") so legacy single-filter config under
"filter_column" is ignored; update the guard around configured_filter_columns
(the block that computes configured_filter_columns and force_trame) to also
check display_options.get("filter_column") and treat a truthy "filter_column"
the same as a configured_filter_columns value (i.e., cause the function to
return False/short-circuit), ensuring both "filter_columns" and legacy
"filter_column" are honored.
---
Nitpick comments:
In `@tests/test_frame.py`:
- Around line 1159-1166: Extract the duplicated display-options dict and
monkeypatch pattern into a single helper or pytest fixture (e.g.,
patch_display_options or display_options_fixture) and use that helper in tests
instead of repeating the dict + monkeypatch.setattr calls; update the locations
that currently call the inline options and monkeypatch.setattr (the blocks
around the options variable and
monkeypatch.setattr("cytodataframe.frame.get_option", ...)) to call the new
helper/fixture so future changes to display options are made in one place.
- Around line 1007-1009: Replace brittle exact-string assertions against
control.children[0].value (e.g., "stroke='#dc2626'", "y1='6.00'", "y2='22.00'")
with semantic checks: assert the SVG contains the threshold line (check for the
stroke color attribute value rather than exact quoting/spacing) and extract the
numeric y1/y2 values from the element string, then assert they lie within
expected bounds or within a small tolerance range. Apply the same change to the
similar assertions around lines 1089-1096 so tests validate presence and
approximate position of the threshold line instead of exact serialized
formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e067dba-9a14-4dbb-a392-18b0272fc6cc
📒 Files selected for processing (5)
docs/src/examples/cytodataframe_at_a_glance.ipynbdocs/src/examples/cytodataframe_at_a_glance.pypyproject.tomlsrc/cytodataframe/frame.pytests/test_frame.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/src/examples/cytodataframe_at_a_glance.py
There was a problem hiding this comment.
Pull request overview
This PR enhances CytoDataFrame’s notebook display by adding per-column (and multi-column) interactive range filters, including an overlaid distribution plot with optional threshold indicators, and updates documentation/pre-commit config to support the new display behavior.
Changes:
- Add interactive filter range sliders (single or multi-column via accordion) with SVG distribution overlays and optional threshold markers.
- Apply widget-selected filter ranges to notebook HTML rendering (including correct handling relative to pandas display truncation).
- Update docs/README notes about pandas display truncation + bump pre-commit tool versions.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/cytodataframe/frame.py |
Implements filter slider state, UI controls, distribution/threshold rendering, and filtering integration into HTML/table rendering. |
tests/test_frame.py |
Adds unit tests for slider state updates, filtering behavior, distribution/threshold rendering, and notebook/trame rendering paths. |
docs/src/examples/cytodataframe_at_a_glance.py |
Adds an example demonstrating display_options["filter_columns"]. |
README.md |
Documents notebook/widget row truncation behavior (ellipsis row) aligned with pandas display settings. |
pyproject.toml |
Adds a Ruff per-file ignore for the docs example and minor formatting cleanup. |
.pre-commit-config.yaml |
Bumps pyproject-fmt and ruff-pre-commit revisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/cytodataframe/frame.py (1)
4885-4889: Keep traceback context in trame fallback logging.Line 4885 intentionally falls back, but the current log message drops stack details, which makes render failures hard to debug.
🔧 Small logging improvement
- except Exception as exc: - logger.debug( - "Trame widget table render failed, falling back to HTML: %s", - exc, - ) + except Exception: + logger.debug( + "Trame widget table render failed, falling back to HTML.", + exc_info=True, + ) return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cytodataframe/frame.py` around lines 4885 - 4889, The fallback except block that currently logs "Trame widget table render failed, falling back to HTML" in frame.py (around the try/except that handles Trame widget table rendering) drops the traceback; change the logging to include full exception context by using logger.exception(...) or logger.debug(..., exc_info=True) instead of the current logger.debug call so the stack trace is preserved when rendering fails (update the except Exception as exc: handler where the message is emitted).tests/test_frame.py (1)
1032-1052: Strengthen clamping assertion to match the test name.On Line 1032, the test name says “clamps,” but current assertions only verify warning + threshold line presence. Add an x-position check at the domain edge to validate actual clamping behavior.
Proposed assertion tightening
def test_filter_slider_control_warns_and_clamps_out_of_range_threshold( caplog: pytest.LogCaptureFixture, ) -> None: @@ with caplog.at_level(logging.WARNING): _slider, control = cdf._build_filter_slider_control_for_column("FilterScore") @@ assert isinstance(control, widgets.VBox) assert isinstance(control.children[0], widgets.HTML) assert "stroke='#dc2626'" in control.children[0].value + html = control.children[0].value + x_match = re.search(r"x1='([0-9.]+)' y1='[0-9.]+'", html) + assert x_match is not None + x_val = float(x_match.group(1)) + track_right = float( + FILTER_SLIDER_TOTAL_WIDTH_PX - FILTER_SLIDER_READOUT_WIDTH_PX + ) + assert abs(x_val - track_right) < 8.0 assert "outside data range" in caplog.text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_frame.py` around lines 1032 - 1052, The test currently checks for a warning and the presence of the threshold SVG stroke but doesn't assert that the slider indicator was clamped to the data domain; update test_filter_slider_control_warns_and_clamps_out_of_range_threshold to also assert the threshold mark's x-position equals the corresponding domain edge from cdf._custom_attrs["_widget_state"]["filter_ranges"]["FilterScore"] after calling cdf._build_filter_slider_control_for_column("FilterScore"); locate the SVG/HTML in control.children[0].value (the same string where you asserted "stroke='#dc2626'") and add a check that the threshold marker's x attribute (or equivalent position representation in that HTML/SVG string) matches the clamped edge value (min or max) derived from the filter_ranges for "FilterScore".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cytodataframe/frame.py`:
- Around line 1205-1208: The current filtering creates `allowed =
set(in_range.index.tolist())` which drops duplicate index labels; instead
preserve duplicate row identities by building a multiset of allowed labels
(e.g., collections.Counter from `in_range.index`) and then iterate
`active_indices` consuming counts so each occurrence is matched once; replace
the set-based membership logic that assigns `allowed` and the list comprehension
over `active_indices` with a loop that checks and decrements the Counter for
`in_range.index` so duplicate labels remain in the filtered `active_indices`.
---
Nitpick comments:
In `@src/cytodataframe/frame.py`:
- Around line 4885-4889: The fallback except block that currently logs "Trame
widget table render failed, falling back to HTML" in frame.py (around the
try/except that handles Trame widget table rendering) drops the traceback;
change the logging to include full exception context by using
logger.exception(...) or logger.debug(..., exc_info=True) instead of the current
logger.debug call so the stack trace is preserved when rendering fails (update
the except Exception as exc: handler where the message is emitted).
In `@tests/test_frame.py`:
- Around line 1032-1052: The test currently checks for a warning and the
presence of the threshold SVG stroke but doesn't assert that the slider
indicator was clamped to the data domain; update
test_filter_slider_control_warns_and_clamps_out_of_range_threshold to also
assert the threshold mark's x-position equals the corresponding domain edge from
cdf._custom_attrs["_widget_state"]["filter_ranges"]["FilterScore"] after calling
cdf._build_filter_slider_control_for_column("FilterScore"); locate the SVG/HTML
in control.children[0].value (the same string where you asserted
"stroke='#dc2626'") and add a check that the threshold marker's x attribute (or
equivalent position representation in that HTML/SVG string) matches the clamped
edge value (min or max) derived from the filter_ranges for "FilterScore".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12bf4c18-4744-4334-8e73-bf40a68ac01e
📒 Files selected for processing (4)
.pre-commit-config.yamldocs/src/examples/cytodataframe_at_a_glance.ipynbsrc/cytodataframe/frame.pytests/test_frame.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .pre-commit-config.yaml
…taFrame into multi-scroll-filter
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cytodataframe/frame.py (1)
1275-1278:⚠️ Potential issue | 🟠 MajorFilter on row positions, not index labels.
set(in_range.index)collapses duplicate labels, and the later full-frame slice re-applies the result through.loc[...]. On a non-unique index, that can resurrect out-of-range rows or duplicate matches in the rendered table. Keep this selection as row positions or a boolean mask until the final slice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cytodataframe/frame.py` around lines 1275 - 1278, The current code filters active_indices by label membership using set(in_range.index) which collapses duplicates and later re-applies .loc, causing incorrect rows on non-unique indexes; change this to filter by row positions or a boolean mask until the final slice: compute positional indices for the rows in in_range (e.g., use self._frame.index.get_indexer(in_range.index) or np.flatnonzero(self._frame.index.isin(in_range.index)) to produce allowed_positions or a boolean mask) and then filter active_indices against those positions (not labels), leaving the final .loc slice to use the original labels only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cytodataframe/frame.py`:
- Around line 628-633: The code treats an empty list/tuple in configured_many as
"configured" and thus prevents falling back to configured_single (so
{"filter_columns": [], "filter_column":"foo"} yields no filter); modify the
conditional around configured_many so it only takes the first branch when it's a
non-empty sequence (e.g., replace "isinstance(configured_many, (list, tuple))"
with a check that the sequence is non-empty such as "isinstance(configured_many,
(list, tuple)) and len(configured_many) > 0" or simply "configured_many" ),
leaving the subsequent elif branches for configured_many is not None and
configured_single unchanged; reference symbols: configured_many,
configured_single, filter_columns, filter_column.
---
Duplicate comments:
In `@src/cytodataframe/frame.py`:
- Around line 1275-1278: The current code filters active_indices by label
membership using set(in_range.index) which collapses duplicates and later
re-applies .loc, causing incorrect rows on non-unique indexes; change this to
filter by row positions or a boolean mask until the final slice: compute
positional indices for the rows in in_range (e.g., use
self._frame.index.get_indexer(in_range.index) or
np.flatnonzero(self._frame.index.isin(in_range.index)) to produce
allowed_positions or a boolean mask) and then filter active_indices against
those positions (not labels), leaving the final .loc slice to use the original
labels only once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47513feb-107b-4c89-a334-5a6562a4b9cc
📒 Files selected for processing (1)
src/cytodataframe/frame.py
…taFrame into multi-scroll-filter
Member
Author
|
@jenna-tomkinson - I think this is ready for a review. I tried to use some data from cosmicqc and the filter columns here and this is how it currently ends up looking with the NF1 test dataset: |
jenna-tomkinson
approved these changes
Mar 17, 2026
jenna-tomkinson
left a comment
jenna-tomkinson
left a comment
Member
There was a problem hiding this comment.
LGTM! I did leave some feedback regarding how the KDE plots are generated that will likely lead to issues with visualization, like when distributions are skewed they will look flat. This work is a great start and lift!
Member
Author
|
Thanks @jenna-tomkinson ! Things are looking a lot better now after making changes based on your comments: I'll merge this in after committing the changes. |
Co-Authored-By: Jenna Tomkinson <107513215+jenna-tomkinson@users.noreply.github.com>
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.
Description
This PR adds display options for multi-scroll filter selection by column with threshold indicators. It will line up with work needed in
cosmicqc.Some screenshots:
Single:
Multi:
Multi with threshold indication:
What kind of change(s) are included?
Checklist
Please ensure that all boxes are checked before indicating that this pull request is ready for review.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores