Seg-Transaction-Stats #153
Conversation
WalkthroughThis pull request introduces an optional Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as SegTransactionStats
participant M as _calc_seg_stats
C->>S: Instantiate with calc_total (true/false)
S->>M: Call _calc_seg_stats(calc_total)
alt calc_total true
M->>M: Calculate segment metrics
M->>M: Calculate total metrics using ibis.union
M-->>S: Return combined metrics
else calc_total false
M->>M: Calculate segment metrics only
M-->>S: Return segment metrics only
end
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 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:
|
|||||||||
![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 (1)
pyretailscience/analysis/segmentation.py (1)
324-331: Simplify the conditional logicThe implementation could be simplified by avoiding the intermediate
final_metricsassignment.# Calculate metrics for segments segment_metrics = data.group_by(segment_col).aggregate(**aggs) -final_metrics = segment_metrics - if calc_total: total_metrics = data.aggregate(**aggs).mutate({col: ibis.literal("Total") for col in segment_col}) - final_metrics = ibis.union(segment_metrics, total_metrics) + segment_metrics = ibis.union(segment_metrics, total_metrics) total_customers = data[cols.customer_id].nunique() # Cross join with total_customers to make it available for percentage calculation -final_metrics = final_metrics.mutate( +segment_metrics = segment_metrics.mutate(This simplifies the code by avoiding the intermediate variable and makes the flow clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyretailscience/analysis/segmentation.py(7 hunks)tests/analysis/test_segmentation.py(8 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
pyretailscience/analysis/segmentation.py (1)
tests/analysis/test_revenue_tree.py (1)
cols(16-18)
tests/analysis/test_segmentation.py (1)
pyretailscience/analysis/segmentation.py (4)
SegTransactionStats(188-464)df(145-149)df(355-369)df(567-571)
🔇 Additional comments (14)
tests/analysis/test_segmentation.py (9)
54-55: LGTM - Parameter addition for calc_totalThe addition of
calc_total=Trueensures this test continues to work as expected with the total row included.
84-88: LGTM - Parameter addition for calc_totalThe addition of
calc_total=Trueensures this test continues to include the total row in its assertions.
112-116: LGTM - Parameter addition for calc_totalThe addition of
calc_total=Trueensures this test continues to include the total row in its assertions.
139-145: LGTM - Parameter addition for calc_totalThe addition of
calc_total=Trueensures this test continues to include the total row in its assertions.
147-171: Great addition of test for the new functionalityThis new test case properly verifies that when
calc_total=Falseis passed, the total row is excluded from the results. The test has appropriate assertions checking that only segment rows (A, B) are present.
377-377: LGTM - Parameter addition for calc_totalThe addition of
calc_total=Trueensures this test continues to include the total row in its assertions.
421-421: LGTM - Parameter addition for calc_totalThe addition of
calc_total=Trueensures this test continues to include the total row in its assertions.
449-455: LGTM - Parameter addition for calc_totalThe addition of
calc_total=Trueensures this test continues to include the total row for the extra_aggs functionality.
468-472: LGTM - Parameter addition for calc_totalThe addition of
calc_total=Trueensures this test continues to include the total row for multiple extra_aggs tests.pyretailscience/analysis/segmentation.py (5)
197-197: LGTM - Addition of the calc_total parameterThe addition of the
calc_totalparameter with a default value ofFalseis a good choice as it maintains backward compatibility with existing code that doesn't set this parameter explicitly.
209-209: LGTM - Updated docstringThe docstring has been appropriately updated to include documentation for the new parameter.
249-249: LGTM - Updated method callThe call to
_calc_seg_statshas been updated to correctly pass the newcalc_totalparameter.
284-284: LGTM - Addition of the calc_total parameterThe
calc_totalparameter has been properly added to the static method with the same default value for consistency.
293-294: LGTM - Updated docstringThe docstring for the static method has been appropriately updated to include documentation for the new parameter.
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
| @@ -194,6 +194,7 @@ def __init__( | |||
| self, | |||
| data: pd.DataFrame | ibis.Table, | |||
| segment_col: str | list[str] = "segment_name", | |||
| calc_total: bool = False, | |||
There was a problem hiding this comment.
Can you make this default to True please
mayurkmmt
force-pushed
the
feature/seg-transaction-stats
branch
from
bbccdb8 to
af3dca1
Compare
User description
feat: added params to cal total row
PR Type
Enhancement, Tests
Description
Added
calc_totalparameter to include/exclude total row.Updated
_calc_seg_statsto handlecalc_totallogic.Enhanced tests to validate
calc_totalfunctionality.Added a new test to verify exclusion of total row.
Changes walkthrough 📝
segmentation.py
Add `calc_total` parameter and update aggregation logicpyretailscience/analysis/segmentation.py
calc_totalparameter inSegTransactionStats._calc_seg_statsto conditionally include total row.test_segmentation.py
Add and update tests for `calc_total` functionalitytests/analysis/test_segmentation.py
calc_totalparameter.aggregations.
calc_totalfunctionality across various scenarios.Summary by CodeRabbit
New Features
Documentation
Tests