Add highlighting feature for data elements - #30
Conversation
Add a new `highlight` parameter to `UpSetAltair()` that allows users to programmatically highlight specific intersections without requiring interactive hover. Features: - highlight="least": highlights the smallest intersection - highlight="greatest": highlights the largest intersection - highlight=<index>: highlights a specific intersection by index (0-based) - highlight=[indices]: highlights multiple intersections - highlight=None: default hover behavior (unchanged) This addresses the need to specify which sets to highlight in static visualizations or when creating non-interactive charts. Added comprehensive tests and documentation examples demonstrating all highlighting modes.
Replace superficial "does it exist" tests with proper validation tests that verify the CORRECT intersections are highlighted. Changes: - test_highlight_least: now verifies the highlighted intersection has the actual minimum count - test_highlight_greatest: now verifies the highlighted intersection has the actual maximum count - test_highlight_specific_index: now verifies the correct intersection_id is selected for the given index - test_highlight_multiple_indices: now verifies all requested indices map to the correct intersection_ids These tests would now catch bugs like: - Swapped least/greatest logic - Off-by-one errors in indexing - Wrong intersection being highlighted - Using degree instead of count for min/max
Improvements: - Extract _determine_highlighted_intersections() helper function for better testability and code organization - Use Literal types for type safety (only "least" and "greatest" allowed) - Reduce main function complexity from ~330 lines to ~300 lines - Make highlight determination logic more reusable and maintainable No functional changes - all tests pass.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a programmatic highlighting feature to UpSet plots, allowing users to automatically highlight specific intersections based on size ("least"/"greatest"), single index, or multiple indices, as an alternative to the default interactive hover behavior.
Key changes:
- Added
highlightparameter toUpSetAltairfunction with support for multiple highlight modes - Implemented
_determine_highlighted_intersectionshelper function to compute which intersections to highlight - Added comprehensive test coverage for all highlight modes and validation
Reviewed Changes
Copilot reviewed 8 out of 13 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
altair_upset/upset.py |
Added highlight parameter and _determine_highlighted_intersections function for programmatic intersection highlighting |
tests/test_advanced_features.py |
Added 11 test cases covering all highlight modes and error conditions |
docs/examples/advanced_features.rst |
Added documentation with examples for each highlight mode |
| Snapshot JSON files | Updated parameter/view names due to internal changes (param_23→param_33, view_45→view_65, etc.) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if highlight < len(intersections): | ||
| return [intersections.iloc[highlight]["intersection_id"]] | ||
| return [] | ||
| else: # isinstance(highlight, list) | ||
| return [ | ||
| intersections.iloc[i]["intersection_id"] | ||
| for i in highlight | ||
| if i < len(intersections) |
There was a problem hiding this comment.
When an integer index is out of bounds, the function silently returns an empty list. This could lead to confusing behavior where no intersection is highlighted without any indication of why. Consider raising an IndexError or ValueError with a descriptive message instead.
| if highlight < len(intersections): | |
| return [intersections.iloc[highlight]["intersection_id"]] | |
| return [] | |
| else: # isinstance(highlight, list) | |
| return [ | |
| intersections.iloc[i]["intersection_id"] | |
| for i in highlight | |
| if i < len(intersections) | |
| if 0 <= highlight < len(intersections): | |
| return [intersections.iloc[highlight]["intersection_id"]] | |
| raise IndexError( | |
| f"Highlight index {highlight} is out of bounds for intersections of length {len(intersections)}." | |
| ) | |
| else: # isinstance(highlight, list) | |
| invalid_indices = [i for i in highlight if not (0 <= i < len(intersections))] | |
| if invalid_indices: | |
| raise IndexError( | |
| f"Highlight indices {invalid_indices} are out of bounds for intersections of length {len(intersections)}." | |
| ) | |
| return [ | |
| intersections.iloc[i]["intersection_id"] | |
| for i in highlight |
| return [ | ||
| intersections.iloc[i]["intersection_id"] | ||
| for i in highlight | ||
| if i < len(intersections) |
There was a problem hiding this comment.
When a list contains out-of-bounds indices, they are silently filtered out. This could cause confusion if a user expects all specified intersections to be highlighted. Consider raising an error if any index is out of bounds, or at least logging a warning.
| return [ | |
| intersections.iloc[i]["intersection_id"] | |
| for i in highlight | |
| if i < len(intersections) | |
| out_of_bounds = [i for i in highlight if i < 0 or i >= len(intersections)] | |
| if out_of_bounds: | |
| raise ValueError( | |
| f"The following indices are out of bounds for intersections (valid range: 0 to {len(intersections)-1}): {out_of_bounds}" | |
| ) | |
| return [ | |
| intersections.iloc[i]["intersection_id"] | |
| for i in highlight |
| (p for p in params | ||
| if "select" in p and "fields" in p["select"] | ||
| and "intersection_id" in p["select"]["fields"] | ||
| and "value" in p), |
There was a problem hiding this comment.
[nitpick] The generator expression spans multiple lines without proper indentation continuation. Consider formatting it consistently across all test functions or extracting it into a helper function to reduce code duplication (appears in lines 236-240, 282-286, 327-331, 367-371).
Address Copilot feedback from PR #30 to improve error handling: 1. Raise IndexError for out-of-bounds single index (was silent failure) - Before: highlight=999 → silently returns empty list - After: highlight=999 → raises IndexError with clear message 2. Raise ValueError if any list indices are out of bounds (was silent filtering) - Before: highlight=[0, 1, 999] → silently ignores 999 - After: highlight=[0, 1, 999] → raises ValueError listing invalid indices 3. Extract test helper function to reduce code duplication - Added _get_color_selection_param() to eliminate repeated code - Improves test maintainability and readability These changes make the API more Pythonic by failing explicitly rather than silently, helping users catch mistakes early.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 13 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
03e94da to
00e7378
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
I'll analyze this and get back to you. |
Address Copilot feedback from PR #30 to improve error handling: 1. Raise IndexError for out-of-bounds single index (was silent failure) - Before: highlight=999 → silently returns empty list - After: highlight=999 → raises IndexError with clear message 2. Raise ValueError if any list indices are out of bounds (was silent filtering) - Before: highlight=[0, 1, 999] → silently ignores 999 - After: highlight=[0, 1, 999] → raises ValueError listing invalid indices 3. Extract test helper function to reduce code duplication - Added _get_color_selection_param() to eliminate repeated code - Improves test maintainability and readability These changes make the API more Pythonic by failing explicitly rather than silently, helping users catch mistakes early.

No description provided.