feat: changed threshold seg to use ibis #89
Conversation
WalkthroughThe changes add a new dependency ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TS as ThresholdSegmentation
participant IbisOps as Ibis Operations
Client->>TS: Submit df, thresholds, segments
TS->>TS: Validate lengths & uniqueness of thresholds and segments
TS->>IbisOps: Filter zero-value customers using ibis operations
IbisOps-->>TS: Return filtered data
TS->>TS: Construct case statement for segment assignment
TS->>IbisOps: Combine segments via ibis.union
TS-->>Client: Return segmented DataFrame
Possibly related PRs
Suggested labels
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
pyretailscience/segmentation.py:76
- Changing the
segmentsparameter from a dictionary to a list could cause issues if the rest of the codebase expectssegmentsto be a dictionary. Please ensure that this change does not break any existing functionality.
segments: dict[any, str],
pyretailscience/segmentation.py:80
- The
zero_segment_idparameter was removed. Please ensure that this change does not break any existing functionality.
zero_segment_id: str = "Z",
Codecov ReportAttention: Patch coverage is
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_segmentation.py (2)
6-9: Consider completing the migration to ColumnHelper.The code imports both
get_optionandColumnHelper, butget_optionis still used in some places. For consistency, consider migrating all column references to useColumnHelper.
32-42: Maintain consistency in column references.Line 40 still uses
get_optionwhile surrounding code usesColumnHelper. For consistency, consider updating this to useColumnHelper.- f"customers_{get_option('column.suffix.percent')}": [0.6, 0.4, 1.0], + f"customers_{cols.suffix_percent}": [0.6, 0.4, 1.0],pyretailscience/segmentation.py (1)
70-150: Well-structured ibis integration with room for optimization.The changes effectively integrate ibis for data processing with proper validation. However, the threshold uniqueness check can be simplified.
- if set(thresholds) != set(thresholds): + if len(set(thresholds)) != len(thresholds):The current check
set(thresholds) != set(thresholds)will always evaluate toFalse. The suggested change correctly identifies duplicate thresholds by comparing the length of the unique values to the original length.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.toml(1 hunks)pyretailscience/segmentation.py(5 hunks)tests/test_segmentation.py(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pre-Commit
🔇 Additional comments (5)
tests/test_segmentation.py (2)
20-24: LGTM!The test data setup correctly uses ColumnHelper for column references and aligns with the segment_name changes.
111-283: LGTM!The test cases thoroughly cover the ThresholdSegmentation functionality including:
- Basic segmentation
- Edge cases (single customer, zero values)
- Error cases (invalid thresholds, segments)
- The changes align well with the ibis integration.
pyretailscience/segmentation.py (2)
177-179: LGTM!The thresholds and segments are correctly defined as lists and align with the industry standard definition (80/20 rule) mentioned in the docstring.
192-192: LGTM!The default segment_col change to "segment_name" maintains consistency with the rest of the codebase.
pyproject.toml (1)
23-23: Verify ibis-framework version compatibility.The addition of ibis-framework with duckdb extra is appropriate for the changes. Let's verify the version compatibility.
✅ Verification successful
🌐 Web query:
What is the latest version of ibis-framework and are there any known compatibility issues with version 9.5.0?Length of output: 3764
Ibis-framework version compatibility verified
The web query confirms that version 9.5.0 is the latest release (September 11, 2024) and that it is appropriate for the recent changes. The only noted issue is a Windows installation problem via Poetry (requiring long path support), which is a known workaround rather than a compatibility blocker.
No modifications are needed in the
pyproject.toml.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_segmentation.py (1)
289-297: Consider adding more test cases for error handling.The test for empty DataFrame with errors is good, but consider adding test cases for:
- Invalid segment names
- Duplicate segment names
- Edge cases with zero-value customers
pyretailscience/segmentation.py (2)
70-151: Excellent implementation ofibisintegration.The refactoring to use
ibisoperations improves performance and maintainability. The use of window functions and case statements is particularly well done.Consider adding caching for the computed DataFrame to improve performance for repeated access.
192-201: Consider adding type hints for return values.Add return type hints to improve code clarity:
- def __init__(self, data: pd.DataFrame | DuckDBPyRelation, segment_col: str = "segment_name") -> None: + def __init__(self, data: pd.DataFrame | DuckDBPyRelation, segment_col: str = "segment_name") -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.toml(1 hunks)pyretailscience/segmentation.py(5 hunks)tests/test_segmentation.py(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pyretailscience/segmentation.py
[warning] 57-58: pyretailscience/segmentation.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 64-64: pyretailscience/segmentation.py#L64
Added line #L64 was not covered by tests
🔇 Additional comments (5)
tests/test_segmentation.py (2)
6-10: LGTM! Good refactoring to useColumnHelper.The transition from using
get_optiontoColumnHelperfor column references improves code readability and maintainability by centralizing column name definitions.
20-24: Verify test coverage for segment name changes.The test case correctly validates the segment name calculations, but we should ensure all edge cases are covered.
Run the following script to check test coverage for segment name handling:
Also applies to: 32-37
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for segment name handling # Search for test cases involving segment name rg -A 5 'segment_name.*=.*' tests/Length of output: 4220
Test coverage for segment name edge cases is comprehensive.
The test cases intests/test_segmentation.pyvalidate multiple scenarios including:
- Standard segmentation with labels like
"A","Total","High", and"Low".- Single-customer segmentation, ensuring labels like
"Heavy"and"Light"are correctly applied.- Handling of zero spend customers with both
"include_with_light"and"separate_segment"settings, covering outcomes such as"Heavy","Light","Medium", and"Zero".- Error handling when required columns are missing.
Given these checks across different configurations, the edge cases for segment name changes appear to be well covered.
pyretailscience/segmentation.py (3)
5-14: LGTM! Good dependency management.The addition of
ibisframework and reorganization of imports improves code structure.
98-103: Good addition of validation checks.The validation for unique thresholds and matching segments improves robustness.
57-64: Add test coverage forExistingSegmentation.Static analysis indicates these lines are not covered by tests.
Would you like me to generate test cases for the
ExistingSegmentationclass to improve coverage?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-58: pyretailscience/segmentation.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 64-64: pyretailscience/segmentation.py#L64
Added line #L64 was not covered by tests
PR Type
Enhancement, Tests
Description
Refactored segmentation logic to use
ibisfor data processing.Replaced
segment_idwithsegment_namefor simplicity.Updated tests to align with new segmentation logic.
Added
ibis-frameworkdependency to project configuration.Changes walkthrough 📝
test_segmentation.py
Updated tests for `ibis`-based segmentation logictests/test_segmentation.py
segment_idwithsegment_namein tests.ColumnHelperfor column references.ibis-based segmentation.segment_id.segmentation.py
Refactored segmentation logic with `ibis` integrationpyretailscience/segmentation.py
ibisfor data processing.segment_idand simplified segment handling.ibis-specific data transformations for segmentation.pyproject.toml
Added `ibis-framework` dependencypyproject.toml
ibis-frameworkdependency withduckdbextras.Summary by CodeRabbit
New Features
ibis-frameworkfor enhanced data analysis.Refactor
Tests