[BUG] adorn_pct_formatting, adorn_ns and adorn_rounding overwrite a numeric first column

[dylanpulver]

PR Description

A tabyl on a numeric grouping variable comes back with its row labels rewritten as data: cyl returns as 400.0%, 600.0%, 800.0% rather than 4, 6, 8. Nothing warns.

adorn_totals and adorn_percentages skip the first column as a row identifier. This applies that exclusion at the three sites that did not.

Constraint: the identifier is positional and belongs to the frame being adorned, so adorn_ns reads it from df and treats every numeric column of ns as a count. R janitor does the same, restoring column index 1 of dat and never deriving a column set from a user-supplied ns.

Rejected: a dtype or name heuristic for identifiers. It would disagree with adorn_totals on the same frame, and R janitor scopes the whole family to "all numeric columns besides the initial column".

Residual limitation: on a plain DataFrame whose first column holds data, adorn_rounding stops rounding it. That is the tradeoff adorn_totals and adorn_percentages make today.

Measurement: 1284 passed against 1266 on dev, same pre-existing failures, doctests identical. Each finding has a test that fails when its hunk is reverted on its own. One test is a pin rather than new coverage: the plain-DataFrame rounding test passes either way and records intent.

This PR resolves #1676.

PR Checklist

1. PR in from a fork off your branch.
2. Add a line to CHANGELOG.md under the latest version header.

[codecov]

Codecov Report

❌ Patch coverage is 95.94595% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.02%. Comparing base (901f4b3) to head (57a8de4).
⚠️ Report is 218 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1677      +/-   ##
==========================================
- Coverage   87.56%   86.02%   -1.55%     
==========================================
  Files          95      125      +30     
  Lines        6819     9993    +3174     
==========================================
+ Hits         5971     8596    +2625     
- Misses        848     1397     +549     

🚀 New features to boost your workflow:

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

[samukweku]

@ericmjl thoughts?

[samukweku]

@dylanpulver did a first pass on your PR and discovered a number of issues that I would like you to have a look at:

• The PR fixes the reported tabyl() bug, but it applies a simple rule everywhere:

“The first column is always the row label, so never process it.”

That works for a normal tabyl:

cyl | gear_3 | gear_4
----+--------+--------
4 | ... | ...
6 | ... | ...

Here cyl really is an identifier. The problem is that the code assumes every related
DataFrame has that same shape.

1. adorn_ns can silently omit counts

adorn_ns accepts a separate ns DataFrame containing the original counts.

The PR excludes the first column of ns, assuming it is the row identifier:

ns = {
"yes": [10],
"no": [5],
}

But in this example, yes is actual data—not an identifier. The PR skips it because it
happens to be first.

So this:

df = pd.DataFrame({
"id": ["A"],
"yes": [0.5],
"no": [0.5],
})

ns = pd.DataFrame({
"yes": [10],
"no": [5],
})

can produce:

id | yes | no
---+------+---------
A | 50% | 50% (5)

The yes count is missing, and no error is raised. That is why I ranked it highest:
the output looks valid but is incomplete.

The fix needs to establish one of these contracts:

• ns must always include the same identifier column as df, and the function should
  validate that; or

• adorn_ns should identify the row-label column from the target df, not blindly from
  ns; or

• the function should explicitly accept a column-selection argument.

2. Empty DataFrames now crash

Before this PR, a DataFrame with no columns simply had no numeric columns to process
and was returned unchanged.

The new code does this:

first_col = df.columns[0]

But an empty DataFrame has no first column:

pd.DataFrame().columns[0]

raises IndexError.

Therefore these formerly harmless calls now fail:

pd.DataFrame().adorn_pct_formatting()
pd.DataFrame().adorn_rounding()
pd.DataFrame().adorn_ns(ns=pd.DataFrame())

This is a straightforward regression. The functions should return unchanged data when
there are no columns, or check for an empty column list before accessing position
zero.

3. Duplicate column names do not follow the claimed positional rule

The PR describes the identifier as positional: “the first column.”

However, the implementation compares column labels:

if col != first_col

Consider:

df = pd.DataFrame(
[[1, 2, 0.5]],
columns=["value", "value", "ratio"],
)

The first "value" is the row identifier, but the second "value" is a data column.
Because both have the same label, both are excluded.

So the implementation behaves as though the rule were:

“Ignore every column with the same name as the first column.”

That is different from:

“Ignore only column position zero.”

If duplicate column labels are intentionally unsupported, the project should validate
or document that. If they are supported, the code needs positional selection.

4. Broader behavior change for ordinary DataFrames

Previously, adorn_rounding, adorn_pct_formatting, and adorn_ns processed numeric
columns regardless of position.

After this PR:

df = pd.DataFrame({
"year": [2020, 2021],
"value": [1.234, 2.345],
})

adorn_rounding() leaves year untouched because it assumes year is a row label.

That is correct for a tabyl, but not necessarily for a normal DataFrame where the
first column is ordinary numeric data. The PR acknowledges this for rounding, so it
is not an undiscovered bug, but it is an API behavior change that should be
deliberate and documented clearly.

[dylanpulver]

@samukweku thanks for the pass, and noted you pulled in @ericmjl.

Three of the four were bugs we introduced. All four are fixed in 22f4049, rebased onto current dev.

1. adorn_ns dropping counts

Our bug, and the ranking was right. Fixed with your second option: the row identifier is read from the target df, not from ns.

R janitor puts it there. When a caller supplies their own ns, adorn_ns() never derives a column set from it:

if (custom_ns_supplied & rlang::dots_n(...) == 0) {
  dont_adorn <- 1L
} else if (rlang::dots_n(...) == 0) {
  cols_to_adorn <- numeric_cols
  dont_adorn <- setdiff(1:ncol(dat), cols_to_adorn)
  dont_adorn <- unique(c(1, dont_adorn)) # always don't-append first column
}

numeric_cols is computed only inside the !custom_ns_supplied branch, and dont_adorn <- 1L indexes dat. The identifier belongs to the frame being adorned.

Your first option is what R enforces, via if (custom_ns_supplied & !identical(dim(ns), dim(dat))) stop(...). That check does not port here. Our adorn_ns aligns rows by position and supports an ns shorter than df on purpose, which test_adorn_ns_skips_totals_row covers. A dimension check would reject that chain. Your third option adds API surface this bug does not require. R's ... tidyselect argument is worth having, as its own change.

Two tests:

• test_adorn_ns_counts_frame_without_identifier_column is your example verbatim. result.iloc[0]["yes"] == "50.0% (10)" is the assertion that fails without the fix. The no and id assertions pass either way and sit there as regression guards.
• test_adorn_ns_identifier_comes_from_target_frame runs the inverse, an ns whose column order puts a data column at position 0 and a numeric id further along. The no and id assertions both fail without the fix. yes is a guard.

2. Empty DataFrames

Our regression, guarded. adorn_totals, adorn_percentages, adorn_title crash the same way on dev today from the same df.columns[0] line, so the guard went in across the family.

test_adorn_on_dataframe_with_no_columns is parametrized over six entry points. All six fail against this PR's previous head. The three named above also fail against dev, so those are older bugs rather than regressions from this PR.

3. Duplicate column labels

Our bug. The description said positional and the code compared labels. Selection is positional now, everywhere in the family, which is what R does on integer indices:

numeric_cols <- which(vapply(dat, is.numeric, logical(1)))
numeric_cols <- setdiff(numeric_cols, 1) # assume 1st column should not be included so remove it from numeric_cols

That idiom appears with its comment intact in adorn_percentages.R, adorn_rounding.R, adorn_pct_formatting.R. tidyselect::eval_select() returns positions too, so nothing in the R family looks a label up.

A _numeric_data_positions helper returns integer positions and writes go through DataFrame.isetitem. Label-keyed writes were unsound for duplicates in any case: df[col] = df[col].apply(...) hands .apply a DataFrame rather than a Series when col repeats.

Three tests:

• test_adorn_pct_formatting_duplicate_column_labels uses your example. result.iloc[0, 1] == "200.0%" and isinstance(result.iloc[0, 1], str) both fail without the fix, as do the two assertions on position 0.
• test_adorn_rounding_duplicate_column_labels uses 1.25 / 2.55 / 0.55 rather than whole numbers, so the position-1 assertion is 2.6 against an unrounded 2.55. On integers that assertion passes either way, which is exactly what the rounding test in the first round of this PR did.
• test_adorn_totals_duplicate_column_labels covers the totals row summing the second value column.

4. Documented behavior change

Every function in the family states the rule in its docstring now, in the same words: position 0 is the row identifier and is not modified even when numeric. On a DataFrame that is not a frequency table, a numeric first column is left alone. adorn_rounding carries a doctest of your year / value example showing year coming back untouched.

test_adorn_rounding_leaves_numeric_first_column_of_plain_dataframe passes with or without the fix. It records intent rather than adding coverage.

The helper being shared means adorn_totals and adorn_percentages moved with the rest. Both carried the label comparison and the empty-frame crash. Behavior is otherwise unchanged and their existing tests pass untouched. Say the word if you want those two split into a separate PR.

Verification

Rebased onto dev at 862b221, only CHANGELOG.md conflicted. Against a clean dev baseline on Python 3.12: 1284 passed against 1266, with zero new failures. The failures that remain are the pre-existing ones in test_conditional_join plus the polars clean_names tests. Doctest results are identical to the baseline. Reverting each hunk on its own fails only the tests for that finding.

The docs job failure is the gh-pages preview deploy returning 403 for fork PRs. That job failed the same way on the previous commit.

[samukweku]

@dylanpulver, thanks for addressing the first review. I did a fresh pass against 22f4049 and found one remaining correctness issue that should be fixed before merge.

adorn_ns silently reuses the first count when labels are duplicated

The positional handling works in adorn_totals, adorn_percentages, adorn_pct_formatting, and adorn_rounding, but adorn_ns converts the numeric columns of ns into a one-position-per-label dictionary:

ns_positions = {}
for pos in _numeric_positions(ns):
    ns_positions.setdefault(ns.columns[pos], pos)

setdefault retains only the first position for a repeated label. Every matching column in df then reads that same count position, so duplicate labels produce plausible-looking but incorrect counts.

Explicit ns example

df = pd.DataFrame(
    [["A", "50.0%", "50.0%"]],
    columns=["id", "yes", "yes"],
)
ns = pd.DataFrame([[10, 20]], columns=["yes", "yes"])

result = df.adorn_ns(ns=ns)

Actual:

id  yes         yes
A   50.0% (10)  50.0% (10)

The second count should come from the second yes column:

id  yes         yes
A   50.0% (10)  50.0% (20)

Stored-counts pipeline

The same problem occurs with _original_counts, and can cause the row identifier itself to be used as the count:

df = pd.DataFrame([[100, 10, 20]], columns=["x", "x", "x"])

result = (
    df.adorn_percentages("row")
    .adorn_pct_formatting()
    .adorn_ns()
)

Actual:

x    x            x
100  33.3% (100)  66.7% (100)

Expected:

x    x           x
100  33.3% (10)  66.7% (20)

This matters because the PR and changelog explicitly establish positional duplicate-label support. On dev, these cases raise an ambiguity error; this change instead returns silently corrupted output.

Please either:

• align ns by position when it has the same schema as df, and define occurrence-aware matching for custom ns; or
• reject ambiguous duplicate-label inputs to adorn_ns with a clear ValueError.

Please also add regression coverage for both an explicit duplicate-label ns and the full stored-counts pipeline above.

[dylanpulver]

Fixed, and both your reproductions now give the output you specified.

adorn_ns matched counts by label through a dict that kept one position per label, so every column of df sharing a label read the same count. Two rules now:

• When ns shares the column axis of df, counts are matched straight across by position. That is the stored-counts path, where _original_counts is a copy of the frame adorn_percentages was handed, identifier column included. Label matching there let the identifier stand in as a count, which is the [[100, 10, 20]] case.
• Otherwise counts are matched by label, and a repeated label is consumed occurrence by occurrence, so the k-th column of df carrying a label reads the k-th count column carrying it. That is the explicit ns case.

Your two cases:

id  yes         yes
A   50.0% (10)  50.0% (20)

x    x           x
100  33.3% (10)  66.7% (20)

Regression coverage for both, test_adorn_ns_duplicate_labels_in_explicit_ns and test_adorn_ns_duplicate_labels_in_stored_counts_pipeline. I checked they fail on the previous commit and pass on this one, so neither can go green by accident.

56 tests in test_adorn.py pass, plus the doctests. ruff and pydoclint are clean. Also rebased on dev, the conflict was the CHANGELOG entry only.