feat: add % of Stores (numeric distribution) metric

[murray-ds]

Summary

• Add PctOfStores class in pyretailscience/metrics/distribution/pct_of_stores.py that computes the percentage of stores selling each product (numeric distribution)
• Add ratio_metric utility in pyretailscience/metrics/base.py for safe division with NaN on zero denominator
• Refactor Acv to follow updated conventions: group_by → group_col, keyword-only params, unconditional validate_columns, input handling before parameter validation
• Add ColumnHelper guideline to CLAUDE.md and add product_id to ColumnHelper
• Add documentation entries in docs/metrics.md and docs/api/metrics/distribution.md
• Consolidate duplicate tests in test_date.py and test_options.py using pytest.mark.parametrize
• Adopt ColumnHelper in test_acv.py for consistency with new guideline

Test plan

• 17 PctOfStores tests pass covering: basic calculation, 100% edge cases (all stores, single store), empty data, missing columns, invalid types, group_col, within_group, both pandas/ibis inputs, custom product_col (str and list), deduplication
• All ACV tests updated and pass with ColumnHelper and group_col
• Full test suite passes
• Lint and format clean

🤖 Generated with Claude Code

[claude]

Claude finished @murray-ds's task in 2m 21s —— View job

---

PR Review: feat: add % of Stores (numeric distribution) metric

• Gather context and understand request
• Read changed files
• Review code quality, bugs, performance, security, and test coverage
• Post review feedback

---

Overall this is a clean, well-structured PR. The PctOfStores implementation is correct, tests are thorough, and the refactoring work is consistent with project conventions. A few findings worth addressing:

---

pyretailscience/metrics/distribution/pct_of_stores.py

count() vs nunique() (minor clarity issue)

pct_of_stores.py:88-90

per_group = store_product.group_by(group_cols).aggregate(
    **{agg_stores_col: _[store_id_col].count()},
)

.count() is functionally correct here because store_product is already .distinct()-ed on [store_id_col, *group_cols], making each row unique. However, .nunique() would be more semantically explicit — a reader shouldn't have to trace back to line 85 to understand why .count() is safe. The within_group total-stores calculation on line 95 already uses .nunique(), so this inconsistency is a bit surprising at first glance.

Fix this →

Temp column name collision risk (low probability, worth noting)

pct_of_stores.py:17

_TEMP_TOTAL_STORES = "__prs_temp_total_stores__"

The double-underscore + prefix convention is a reasonable sentinel, but if a user's dataset happens to have a column named __prs_temp_total_stores__, it would cause a conflict when joining or dropping. This is very unlikely but worth a brief comment explaining the naming choice.

---

pyretailscience/metrics/base.py

scale type annotation could be more precise

base.py:16

def ratio_metric(
    numerator: ir.NumericValue,
    denominator: ir.NumericValue,
    scale: float = PERCENTAGE_SCALE,
) -> ir.FloatingValue:

The denominator type annotation says ir.NumericValue, but the function also works (and is used in pct_of_stores.py) with a scalar expression returned by .nunique(). ir.NumericValue is the correct base class for both column and scalar numeric expressions in ibis, so this is technically fine — just worth confirming the annotation is broad enough intentionally.

---

pyretailscience/utils/date.py

Behavioural breaking change: mixed timezone inputs now raise TypeError

date.py:193-196

if start_is_naive != end_is_naive:
    msg = "start_date and end_date must have matching timezone awareness. Got naive and aware (or vice versa)."
    raise TypeError(msg)

Previously, mixed inputs (e.g., a naive start_date + tz-aware end_date) were silently accepted and treated as naive. Now they raise a TypeError. This is an improvement, but it's a breaking change for any caller passing mismatched types. There are no tests covering this new error path. If this function is part of the public API, a test like:

def test_mismatched_timezone_raises(self):
    with pytest.raises(TypeError, match="matching timezone awareness"):
        find_overlapping_periods(
            datetime.datetime(2022, 1, 1),
            datetime.datetime(2023, 1, 1, tzinfo=datetime.timezone.utc),
        )

would document and protect this new behaviour. Fix this →

np import removal

The removal of import numpy as np / import pandas as pd and the switch from np.arange to range is a nice cleanup. range is semantically correct here since only integer years are iterated.

---

Codecov partial branch

Codecov reports 1 partial branch in pct_of_stores.py (98.03% patch coverage). Partial coverage typically means one side of a boolean short-circuit or a conditional expression isn't exercised. Given the test suite is otherwise exhaustive, this is probably a minor branch in the within_group logic. Worth identifying and filling — but not a blocker.

---

Tests

The test suite is excellent overall:

• All public constructor branches are covered (pandas, ibis, invalid type, missing columns, overlap validation)
• within_group=True/False semantics are clearly separated with inline comments explaining the expected arithmetic
• test_duplicate_store_product_not_double_counted directly validates the .distinct() deduplication, which is the most important correctness invariant
• Parametrize is used appropriately throughout (replacing the old duplicated test methods in test_date.py and test_options.py)

One minor note: test_empty_dataframe creates a DataFrame without a unit_spend column — this is intentional and correct since PctOfStores doesn't require it, but a brief comment would help future readers.

---

Summary

Area	Status
Core logic correctness	✅
Edge cases (empty, single store, dedupe, within_group)	✅
API consistency with Acv conventions	✅
Test quality	✅
Missing test for new TypeError in date.py	⚠️
.count() vs .nunique() semantic clarity	Minor
Codecov partial branch	Minor

[github-actions]

🚀 Cloudflare Pages Deployment

✅ Preview deployed successfully!

Preview URL: https://52253f13.pyretailscience-docs.pages.dev
Deployment ID: 52253f13-c8b9-48be-8022-fe207ae22cad

---

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

[codecov]

Codecov Report

❌ Patch coverage is 98.85057% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...etailscience/metrics/distribution/pct_of_stores.py	98.03%	0 Missing and 1 partial ⚠️

Files with missing lines	Coverage Δ
pyretailscience/metrics/base.py	100.00% <100.00%> (ø)
pyretailscience/metrics/distribution/acv.py	96.77% <100.00%> (ø)
pyretailscience/options.py	98.23% <100.00%> (+<0.01%)	⬆️
pyretailscience/utils/date.py	96.10% <100.00%> (+0.71%)	⬆️
...etailscience/metrics/distribution/pct_of_stores.py	98.03% <98.03%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.