Skip to content

refactor!: return result keys as columns on a RangeIndex - #639

Merged
murray-ds merged 3 commits into
mainfrom
refactor/key-columns-as-columns
Nov 8, 2026
Merged

murray-ds merged 3 commits into
mainfrom
refactor/key-columns-as-columns

Conversation

@murray-ds

Copy link
Copy Markdown
Contributor

Closes #488

Every analysis and segmentation result returns its key and grouping columns as ordinary columns on a default RangeIndex. Nothing is keyed by the index any more, so results join with merge(..., on="customer_id"), and the plots that need row labels take a y_col naming that column.

Breaking changes

Result Key columns now
PurchasesPerCustomer.df, DaysBetweenPurchases.df customer_id, sorted by it
TransactionChurn.df transaction_number, sorted by it
RFMSegmentation.df, ThresholdSegmentation.df, HMLSegmentation.df, NLRSegmentation.df customer_id, then group_col when given
CrossShop.cross_shop_df group_col (first column)
GainLoss.gain_loss_df group_col when given, then customer_id
GainLoss.gain_loss_table_df one row ungrouped; group_col column when grouped
RevenueTree.df the group_col columns
CohortAnalysis.df min_period_shopped, then one column per period

Code that used right_index=True, .loc[customer_id] or .index on these frames needs to merge on, filter by or read the column instead.

Three changes need a line of their own in the release notes:

  • calc_tree_kpis takes group_col and pairs rows across periods on it. Without group_col, each frame must hold one row, and rows are paired by position. A caller who relied on index labels to keep two stores apart now gets one combined row with no error, so pass group_col.
  • CrossShop counting customers: with value_col="customer_id", agg_func="nunique", the count column is named customer_id_nunique in both frames, since customer_id is now the key column.
  • cohort.plot defaults to y_col="min_period_shopped", so CohortAnalysis.df plots as is. A hand-built matrix with the cohorts in the index needs y_col=None. heatmap.plot defaults to y_col=None (the index) and raises a TypeError naming the non-numeric column, with a hint to pass y_col.

Other changes

  • GainLoss with a categorical group_col no longer gives all-zero rows for unused categories.
  • The customer frames are sorted with order_by in the query.
  • A few private helpers and constants that took more lines than they saved are inlined (_TRANSACTION_NUMBER_COL, _GROUPING_FLAG_PREFIX, CrossShop._generate_default_labels, line._generate_colors, graph_utils._hatches_gen, bar._get_bar_value), and a no-op reset_index in ProductAssociation.df is gone.
  • Docstring examples, the bundled skill's references and example scripts, docs/analysis_modules.md, its SVG script and the affected notebooks follow the new shape. The notebooks were re-executed.

Tests

Tests for every result above compare against expected frames with the key columns as columns. New tests cover the CrossShop name clash, grouped and ungrouped GainLoss tables and plot labels, unused categories, calc_tree_kpis pairing and its errors, and heatmap/cohort y_col. The full suite passes locally (1711) and mkdocs build --strict passed. The integration backends weren't run locally.

Not in this PR

  • GainLoss with a group_col named like an output column (e.g. "new") fails.
  • Grouped GainLoss builds a stores × customers intermediate frame, about 5 s per step at 200 stores and 100K customers. This predates the change.

🤖 Generated with Claude Code

https://claude.ai/code/session_014D9DLjMrprSVmrij28jv39

@murray-ds murray-ds added the major-change Headline change for the release notes label Nov 3, 2026
@claude

claude Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Claude finished @murray-ds's task in 22s —— View job


Review of #639: result keys as columns on a RangeIndex

The change is consistent across modules. Net code is smaller (the diff removes helpers and constants), the breaking changes are documented, and the tests assert against full expected frames. I read the analysis, plots and segmentation source diffs. I didn't run the test suite or the integration backends.

No blocking issues. The points below are minor.

Suggestions

  1. calc_tree_kpis without group_col (revenue_tree.py, _pair_periods)

    • Rows pair by position, so two frames indexed by different store labels silently merge into one row. The PR description already flags this.
    • Because the guard only triggers when a frame has more than one row, the single-row-per-store case never errors. A DeprecationWarning or a stricter error would be safer for a breaking release, but the release-note callout may be enough.
  2. Duplicate y_col validation (plots/cohort.py and plots/heatmap.py)

    • cohort.plot raises its own ValueError for a missing y_col, and heatmap.plot then calls ensure_columns on the same column.
    • Cohort's message adds a useful "pass y_col=None" hint, so this is defensible. Check that the two messages don't disagree if someone calls heatmap.plot directly.
  3. "ABC"[: n] literal (cross_shop.py)

    • It appears twice, once in __init__ and once in the static builder. This is fine given the "no premature abstraction" rule.
    • It silently truncates if group_count ever exceeds 3. Validation currently caps it at 3, so there's no bug today.
  4. GainLoss ungrouped plot (gain_loss.py)

    • plot_data.set_axis([""]) blanks the tick label, and kwargs.pop("x_col", None) silently ignores a caller-supplied x_col. Both are documented in the docstring.
    • The set_axis workaround is slightly fragile. If bar.plot later changes how it handles an empty-string label, this breaks quietly. The new plot-label tests do cover it.
  5. GainLoss._process_gain_loss restructure

    • The else: branch re-indents the whole apply block, which inflates the diff.
    • The row-wise apply is a pre-existing performance issue, and the PR already calls out the stores × customers intermediate frame. No change is needed here.
  6. customer_id dtype restore (gain_loss.py)

    • reset_index().astype({cols.customer_id: customer_id_dtype}) guards against dtype drift. This is good.
    • Check it behaves with a nullable or categorical customer_id.

Positives

  • Unused categories no longer produce all-zero GainLoss rows (observed=True).
  • heatmap.plot now raises a TypeError that names the non-numeric columns and hints at y_col, instead of failing obscurely later.
  • Sorting in the query with order_by keeps ordering deterministic across backends.
  • The skill references, the example scripts and the docstring Examples: were updated together, as CLAUDE.md requires.
  • The CrossShop name clash (customer_id_nunique) is handled and has a test.

Docs

The docs/analysis_modules.md and notebook updates look consistent with the new shapes.

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

🚀 Cloudflare Pages Deployment

✅ Preview deployed successfully!

Preview URL: https://d4ba09a4.pyretailscience-docs.pages.dev
Deployment ID: d4ba09a4-4918-49d3-b52e-bfecf870abaf


This preview will be updated automatically when you push new changes to this PR.

@codecov

codecov Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
.../using-openretailscience/scripts/example_cohort.py 100.00% <100.00%> (ø)
...using-openretailscience/scripts/example_heatmap.py 100.00% <100.00%> (ø)
...nretailscience/scripts/example_hml_segmentation.py 100.00% <ø> (ø)
...nretailscience/scripts/example_nlr_segmentation.py 100.00% <ø> (ø)
...ience/scripts/example_segment_transaction_stats.py 100.00% <ø> (ø)
...lscience/scripts/example_threshold_segmentation.py 100.00% <ø> (ø)
openretailscience/analysis/cohort.py 98.36% <100.00%> (+0.74%) ⬆️
openretailscience/analysis/cross_shop.py 100.00% <100.00%> (ø)
openretailscience/analysis/customer.py 100.00% <100.00%> (ø)
openretailscience/analysis/gain_loss.py 100.00% <100.00%> (+5.19%) ⬆️
... and 12 more

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

murray-ds and others added 3 commits November 8, 2026 15:54
Every analysis and segmentation result now carries customer_id, group_col
and other key columns as ordinary columns instead of the index, so results
merge on="customer_id". heatmap.plot and cohort.plot take a y_col naming
the row-label column.

Also inlines a few private helpers and constants that cost more lines
than they saved, and sorts the customer frames in the query.

Closes #488

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014D9DLjMrprSVmrij28jv39
dict(zip(customer_id, segment_name)) collapsed duplicate customers and
ignored the per-period value columns, so compare whole frames instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014D9DLjMrprSVmrij28jv39
@murray-ds
murray-ds force-pushed the refactor/key-columns-as-columns branch from 82813a2 to f9db574 Compare November 8, 2026 14:55
@murray-ds
murray-ds merged commit d3ce288 into main Nov 8, 2026
7 checks passed
@murray-ds
murray-ds deleted the refactor/key-columns-as-columns branch November 8, 2026 15:06

This branch was successfully deployed

1 active deployment
preview — f9db574c Deployed Nov 8, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major-change Headline change for the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Return key columns as columns in every result; plots index their own copy

1 participant