Skip to content

docs: state each public API's contract (ranges, columns, index, shape, valid values), not backend mechanics #591

Closed

Description

@murray-ds

Noticed while watching coding agents work from the installed package, then audited module by module. Checked on main @ c5f4b2a; each claim below was confirmed by running a small snippet.

What: #585 is one case of a wider pattern. Docstrings explain how something is computed, such as the backend, ibis calls or layout internals, but leave out what the caller needs to use the result. That means: the range or base of a value, its units, the output's columns and index, its shape, which rows it includes, the valid values of string parameters, and the input dtypes it requires. A few docstrings state things the code contradicts.

Ask: rewrite these to state the contract (what goes in, what comes out) and drop the mechanics. Where a docstring contradicts the code, fix whichever one is wrong. Links: L = line on main @ c5f4b2a.

Analysis

GainLoss (gain_loss.py)

  • The masks are documented as "boolean masks or lists aligned to df.index" (L3, L20, L41, L47-52), but Python lists raise TypeError: unsupported operand type(s) for |: 'list' and 'list'. "Aligned to df.index" also invites set_index('customer_id'), which with repeated ids always fails "p1_index and p2_index should not overlap". Say: one entry per row of df, as a Series on df's index or a numpy array; df needs a unique index.
  • Output is documented only in private _calc_gain_loss. Document on the class:
    • gain_loss_df is indexed by customer ((group_col, customer_id) when grouped), so customer_id is not a column.
    • The 15 columns.
    • The six components sum to focus_diff.
    • Rows that are all zero are dropped.
    • gain_loss_table_df is one row when ungrouped.
  • agg_func: say it takes a pandas groupby aggregation name, case-sensitive.

CohortAnalysis (cohort.py)

  • .df shape is unstated. It is a square matrix: the index is the cohort start (min_period_shopped) and the columns are periods since start (period_since, 0-based). Cells past the end of the data, and cohorts with no customers, are 0, the same as a genuine zero.
  • percentage=True "converts cohort values into retention percentages" (L86-88). The values are fractions (1.0 = 100%), rounded to 2 decimals.
  • agg_func: list the accepted values. The default nunique on the docstring's own aggregation_column="unit_spend" counts distinct spend values, not customers; say to use customer_id with nunique for retention.
  • Say transaction_date must be datetime (see the error-message issue).

CrossShop (cross_shop.py)

  • percent in cross_shop_table_df is a share of the summed value_col, so spend by default, not customers. plot (L242, L253) calls the result "customer segment overlaps". With agg_func="mean", the table sums per-customer means.
  • Document cross_shop_df:
    • its index is the group_col value;
    • groups is a tuple of positional 0/1 flags;
    • entities in no group are kept as "No Groups" and count in the denominator.
  • group_3_col without group_3_val raises ValueError (L155), which __init__'s Raises section doesn't list.

RevenueTree (revenue_tree.py)

  • period_col, "Column identifying the period" (L225-228): say it holds a period label per row, assigned by the caller, and that p1_value/p2_value are matched exactly. A raw transaction-date column matches almost nothing. Both labels must be present in the data.
  • .df index and columns are documented nowhere (the class docstring says only "{metric}_p1"). Document:
    • A single row (RangeIndex) when ungrouped. A CategoricalIndex for one group_col, a MultiIndex for several.
    • Columns {metric}_p1/_p2/_diff/_pct_diff/_contrib for customers, transactions, spend, spend_per_customer, … There is no revenue_*.
    • _pct_diff is a fraction.
    • A group missing from one period gets 0 for it.
  • Private _agg_data claims a ["p1","p2"] index (L273-275); the result has none.

CompositeRank (composite_rank.py)

  • L22-27 and L47-50 describe ibis.least, ibis.greatest, ibis.rank and ibis.row_number. State the contract instead:
    • ties share the lowest rank (1, 2, 2, 4);
    • ignore_ties=True breaks ties arbitrarily;
    • nulls rank last;
    • row order is not guaranteed;
    • composite_rank is a float for mean.

CustomerDecisionHierarchy (customer_decision_hierarchy.py)

  • Say what distance means: 0 = bought by the same customers, 1 = never. With the default exclusion, near products are substitutes.
  • Say that products bought only in multi-item transactions are dropped.
  • Say that distances is ordered by pairs_df[product_col].cat.categories (stated only on a private method, L148-152).

TransactionChurn (customer.py, L239-253)

  • Define the denominator:
    • row N counts customers whose N-th purchase day is at least churn_period days before the last day in the data;
    • churned_pct = churned / (churned + retained);
    • transaction_number is 1-based.

ProductAssociation (product_association.py)

  • group_col (L54-55): say it defines the basket. The default customer_id makes all of a customer's purchases one basket; pass transaction_id for per-transaction baskets. Support differed 0.667 vs 0.25 on the same data.
  • target_item (L56-57) is typed str | float. Say int product ids are rejected (TypeError).
  • uplift uses undefined prob_*; define it as prob_i = occurrences_i / total.

haversine_distance (haversine.py)

  • Replace "pushed to the backend" and "nothing is materialized" with the contract: a column expression in radius units (km by default).

Segmentation

RFMSegmentation (rfm.py): the NTILE and backend wording is #585.

  • .df (L303) "Returns the dataframe with the segment names" is wrong; it has no segment names. List the columns: recency_days, frequency, monetary, r_score/f_score/m_score, rfm_segment, fm_segment. Say it is indexed by customer_id.

ThresholdSegmentation (threshold.py)

  • thresholds (L43-48) says only "unique and match the number of segments". Say they are ascending percentile ranks in (0, 1] and the last must be 1.0. Today [0.5, 0.8] silently leaves the top 20% with segment None, and an unsorted list puts everyone in one segment; whether to validate instead is in the error-message issue.
  • Replace the "percent_rank" and "window" wording (L33-38) with the contract: a customer takes the first segment whose threshold their percentile rank is ≤; ties are broken by customer_id.
  • agg_func (L109): list the accepted values.

HMLSegmentation (hml.py, L156-173)

  • Say that the 50/80 cut points are percentile ranks of customers, not spend values.
  • Define each zero_value_customers mode.
  • Say .df is indexed by customer_id (and group_col).

SegTransactionStats (segstats.py)

  • .df: document
    • RangeIndex, with segment columns as columns;
    • row order unspecified unless sort_by;
    • one row per segment combination present per grouping set;
    • columns left out of a grouping set hold rollup_value;
    • ratios are null when the denominator is 0;
    • customer metrics are absent without a customer column.
  • grouping_sets: say each tuple is one aggregation level, and that the columns it omits are labelled rollup_value.
  • unknown_customer_value (L238-242): "metrics are split into identified, _unknown, and _total variants" is inaccurate. customers and the per-customer ratios have no variants, and plain spend/transactions then cover identified customers only.
  • extra_aggs (L225-227): say which aggregation names are valid.
  • Replace the mechanics:
    • calc_rollup "Adds O(n) extra aggregation passes" (L229-231);
    • sort_by "using ibis .order_by() so the sort runs at the backend" (L253-254).

Utils, options, experimental

  • filter_and_label_by_periods (date.py, L103-111): a date-only end means 00:00 that day, so later times on that day are dropped from a timestamp column. Document it, or make end inclusive of the whole day.
  • label_by_condition (label.py, L277): say the result has only label_col and return_col.
  • CLVStats (clv.py):
    • Replace the Oracle day-granularity explanation (L19-21) with: recency and T are fractional period units from calendar-day differences.
    • Replace the query-count text (L138-142) with the eager/lazy contract.
    • pymc_time_unit (L476-478) contradicts itself.
  • Acv and PctOfStores (experimental/metrics):
    • Acv hardcodes "$MM" whatever the currency.
    • Say stores_pct is 0-100, not a fraction.
    • Say the group columns are columns (RangeIndex) and row order is unspecified.

Plots

  • area.plot (area.py, L33-40): "if None, the index is used", but with group_col set, x_col=None raises KeyError: None. line and scatter handle the same case.
  • cohort.plot (plots/cohort.py, L150-165): describes label placement and colour-bar format. Say instead that with percentage=True the cells must be fractions; 25 renders as "2500%".
  • line.plot (line.py, L206-210): says move_legend_outside and legend_title are ignored whenever a legend shows. That is true only for "end_of_line"; time.py L62-64 has it right.
  • waterfall.plot (waterfall.py):
    • remove_zero_amounts "Drop zero amounts before the net is computed" (L376-377) can't change the net. It hides the zero bars and their labels.
    • "percentage" labels are shares of the net total, so they can exceed 100% or be negative (L388-392).
  • broken_timeline.plot (broken_timeline.py, L508-510): "gets a shorter bar, not a gap" is wrong. Bars are whole periods, and the threshold removes rows, so an empty period becomes a gap.
  • time.plot (time.py, L37-42): say the date column must be datetime. It also adds transaction_period to the caller's df in place (L79); better to copy first.
  • index.plot (index.py, L194): list the agg_func values, which today appear only on get_indexes (L417).
  • bar.plot (bar.py):
    • percentage_by_bar_group needs unique x values (L75-90).
    • sort_order sorts by the first value_col only (L80-82).
  • venn.plot (venn.py, L592-596): say that flag i in groups corresponds to labels[i], that groups must be a tuple, and that percent is shown unscaled.
  • tree_diagram (tree_diagram.py, L174, L526): say percent is in percentage points, not a fraction.
  • apply_chart_chrome and apply_legend (styling_helpers.py): replace the "resize-invariant recipe", _ChromeLayoutEngine and "reflow" wording with what the caller gets.
  • Every plot's x_label/y_label: say that omitting them gives no label, not the column name.

Related: #585 (RFM), #586 (revenue tree contribution formulas), #590 (exports, dir(), help()).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationImprovements or additions to documentation

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions