feat(report): make the effect column selectable (--effect-col) - #34
Open
dchaudhari7177 wants to merge 1 commit into
Open
feat(report): make the effect column selectable (--effect-col)#34dchaudhari7177 wants to merge 1 commit into
dchaudhari7177 wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #30.
Problem
report.pypinned the effect column at module scope (_COL_EFFECT = "formality") and_read_sweep_rowsreadrow[_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 onmain— the cross-model sentiment/verbosity runs and the redosed layer sweeps emit the effect column aseffectrather thanformality:With this PR,
steer-report --dose-csv artifacts/dose_response_sentiment_qwen.csv ... --effect-col effectrenders them.Change
effect_columnis 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(defaultformality), passed straight through_COL_EFFECTbecomesDEFAULT_EFFECT_COLUMN(the CLI needs it to build its--helptext and default), and_SWEEP_COLUMNSnow 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
KeyErrormid-file. The message now also lists what the CSV does carry:Tests
CPU-only, no model and no download — the fixtures are the existing canned CSVs with the header renamed.
test_effect_column_is_selectable— asentimentdose CSV and layer CSV parse correctly witheffect_column="sentiment", and the coherence columns are unaffected.test_effect_column_defaults_to_formality— the samesentimentCSV 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 wordavailable, and the real column all appear in the message.test_cli_effect_col_reports_a_non_formality_sweep— end to end throughsteer-report: the committed M0 artifacts with the header renamedformality→sentimentrender byte-identical markdown to the unrenamed run under the default.test_cli_effect_col_unknown_column_is_reported— the CLI surfaces the namedValueError.Checks
ruff check .,ruff format --check .,mypy src,pytest -qall pass (68 passed).Two notes on running the suite on Windows, neither caused by this change:
mypy srcreports a syntax error insidenumpy/__init__.pyiif numpy happens to be in the environment (it comes in with the optionalreportextra, not with thedevgroup) because the config targetspython_version = "3.11".mypy --python-version 3.12 srcis clean; CI does not install numpy, so it is clean there too.tests/test_cli.pytests that actually render a card fail on Windows under a non-UTF-8 locale, on cleanmainas well as here:build_reportcallsPath.write_textwith noencoding=, 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; withPYTHONUTF8=1the 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
ReportDataand through both renderers and both plots, which is a wider change than this issue asks for — happy to follow up if you want it.