Skip to content

feat(report): make the effect column selectable (--effect-col) - #34

Open
dchaudhari7177 wants to merge 2 commits into
bamdadd:mainfrom
dchaudhari7177:feat/effect-column
Open

feat(report): make the effect column selectable (--effect-col)#34
dchaudhari7177 wants to merge 2 commits into
bamdadd:mainfrom
dchaudhari7177:feat/effect-column

Conversation

@dchaudhari7177

@dchaudhari7177 dchaudhari7177 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Closes #30.

Problem

report.py pinned the effect column at module scope (_COL_EFFECT = "formality") and _read_sweep_rows read row[_COL_EFFECT] directly. A sweep for either of the other two shipped concepts — sentiment, verbosity — could not be reported at all: the missing-columns guard rejected the CSV before parsing, and the only workaround was editing the module.

This already bites the committed artifacts. 14 of the 24 sweep CSVs in artifacts/ cannot be rendered on main — the cross-model sentiment/verbosity runs and the redosed layer sweeps emit the effect column as effect rather than formality:

$ python -c "from pathlib import Path; from steerbench import report; \
    report.load_dose_curve(Path('artifacts/dose_response_sentiment_qwen.csv'))"
ValueError: artifacts/dose_response_sentiment_qwen.csv is missing columns: ['formality']

With this PR, steer-report --dose-csv artifacts/dose_response_sentiment_qwen.csv ... --effect-col effect renders them.

Change

effect_column is threaded through the read path and defaulted to "formality" everywhere, so nothing about current behaviour changes:

  • _read_sweep_rows(path, x_column, effect_column="formality")
  • load_dose_curve(path, effect_column="formality")
  • load_layer_curve(path, x_column="layer", effect_column="formality")
  • build_report(..., effect_column="formality")
  • steer-report --effect-col NAME (default formality), passed straight through

_COL_EFFECT becomes DEFAULT_EFFECT_COLUMN (the CLI needs it to build its --help text and default), and _SWEEP_COLUMNS now holds only the columns that are the same for every concept — seed, repetition, ppl — with the x column and the effect column added per call.

Error path

The requested column is folded into the existing missing-columns check rather than getting a second guard, so a wrong name fails before any row is parsed instead of raising a bare KeyError mid-file. The message now also lists what the CSV does carry:

dose.csv is missing columns: ['verbosity'] (available: ['alpha_norm', 'coeff', 'formality', 'ppl', 'repetition', 'seed'])

Tests

CPU-only, no model and no download — the fixtures are the existing canned CSVs with the header renamed.

  • test_effect_column_is_selectable — a sentiment dose CSV and layer CSV parse correctly with effect_column="sentiment", and the coherence columns are unaffected.
  • test_effect_column_defaults_to_formality — the same sentiment CSV still fails the guard on the default, so the default really is unchanged.
  • test_unknown_effect_column_names_the_available_columns — asserts the requested name, the word available, and the real column all appear in the message.
  • test_cli_effect_col_reports_a_non_formality_sweep — end to end through steer-report: the committed M0 artifacts with the header renamed formalitysentiment render byte-identical markdown to the unrenamed run under the default.
  • test_cli_effect_col_unknown_column_is_reported — the CLI surfaces the named ValueError.

Checks

ruff check ., ruff format --check ., mypy src, pytest -q all pass (68 passed).

Two notes on running the suite on Windows, neither caused by this change:

  • mypy src reports a syntax error inside numpy/__init__.pyi if numpy happens to be in the environment (it comes in with the optional report extra, not with the dev group) because the config targets python_version = "3.11". mypy --python-version 3.12 src is clean; CI does not install numpy, so it is clean there too.
  • The three tests/test_cli.py tests that actually render a card fail on Windows under a non-UTF-8 locale, on clean main as well as here: build_report calls Path.write_text with no encoding=, so the Δ in the side-effects table and the ⚠️ in the trap warnings hit cp1252. I verified this by stashing my changes and re-running. It is a separate bug and I'll send a separate PR for it rather than fold an unrelated fix in here; with PYTHONUTF8=1 the full suite is green locally.

Deliberately out of scope

The rendered card still labels the series generically ("effect (behaviour score)") rather than naming the concept. Doing that properly means carrying the column name into ReportData and through both renderers and both plots, which is a wider change than this issue asks for — happy to follow up if you want it.

report.py hardcoded the effect column as formality, so a sweep of any other
shipped concept (sentiment, verbosity) could not be reported without editing
the module.

Thread effect_column through the CSV reader and both loaders, add it to
build_report, and expose it as steer-report --effect-col. The default stays
"formality", so existing callers and the committed M0 artifacts are
unaffected.

The requested column is folded into the existing missing-columns guard rather
than getting its own check, so a wrong name fails before any row is parsed and
the error now also lists the columns the CSV does carry.

Closes bamdadd#30

@bamdadd bamdadd left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the feature itself is clean: DEFAULT_EFFECT_COLUMN is threaded through _read_sweep_rows into both loaders and build_report, the default is preserved so existing M0 artifacts are unaffected, and it's well covered (2 CLI tests + 3 report tests). pytest/ruff/mypy all green locally.

One change before merge: an invalid --effect-col value surfaces as an uncaught ValueError traceback and exits 1:

ValueError: artifacts/dose_response.csv is missing columns: ['nope'] (available: [...])

The message content is good, but the delivery is inconsistent with how the CLI now handles user input errors. The missing-CSV guard a few lines above goes through parser.error() → clean one-line message, exit 2, and #21/#22/#23 all moved CLI input errors to that pattern. A user typo in --effect-col should do the same.

Please route the bad-column case through parser.error() (either pre-validate the column against the header, or catch the ValueError around the build_report call and re-raise via parser.error), and add/adjust a CLI test asserting exit code 2 and no traceback. After that this is good to merge.

An invalid --effect-col reached the user as an uncaught ValueError from
mid-parse in report.py, exiting 1 with a traceback. The message content was
right, but the delivery was inconsistent with every other CLI input error --
the missing-CSV guards a few lines above, and bamdadd#21/bamdadd#22/bamdadd#23, all go through
parser.error() for a clean one-line message and exit 2.

Pre-validate the column against both sweep headers instead, next to the
existing existence checks. report.sweep_columns() splits the header read out
of _read_sweep_rows so the check does not duplicate the parse; the in-parse
guard stays as the library-level backstop for direct load_dose_curve callers.

Checking both CSVs matters because the column is read from each: a name only
one sweep carries now names which file is short.
@dchaudhari7177

dchaudhari7177 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Good catch — fixed in cbd7b28.

--effect-col is now pre-validated against both sweep headers, right next to the existing existence guards, so a typo gets the same treatment as every other bad argument:

$ steer-report --effect-col nope
steer-report: error: --effect-col 'nope' is not a column of the dose-response CSV artifacts/dose_response.csv (available: alpha_norm, coeff, formality, ppl, repetition, seed)
$ echo $?
2

I went with pre-validation rather than wrapping build_report in a try/except ValueError — that call also runs aggregation and rendering, so catching ValueError around the whole thing would relabel an unrelated internal failure as a user typo. report.sweep_columns() splits the header read out of _read_sweep_rows so the check doesn't duplicate the parse. The in-parse guard stays put as the library-level backstop for anyone calling load_dose_curve directly, which is what test_unknown_effect_column_names_the_available_columns still covers.

Two CLI tests: the existing one now asserts exit 2, the message, and no traceback; a new one covers a column present in the dose sweep but not the layer sweep, since both are read with the same name — that case now names which CSV is short.

Full suite, ruff and mypy green locally.

One note unrelated to this PR: three test_cli.py tests fail on Windows under the default cp1252 encoding — report.py writes the Δ character through Path.write_text with no explicit encoding. PYTHONUTF8=1 works around it. Happy to send a one-line encoding="utf-8" fix separately if that isn't already tracked.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the report effect column selectable (report.py hardcodes 'formality')

2 participants