Fix benchmark fairness, profiling attribution, and reportability semantics#11
Fix benchmark fairness, profiling attribution, and reportability semantics#11lullu57 wants to merge 9 commits into
Conversation
Made-with: Cursor # Conflicts: # scripts/slurm/run_kerneltuner_array.sbatch # scripts/slurm/submit_kerneltuner.sh
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8280b12e93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| combined = pd.concat(rows, ignore_index=True) | ||
| combined["weighted_regret_sum"] = ( | ||
| pd.to_numeric(combined.get("avg_regret_to_best_measured"), errors="coerce").fillna(0.0) | ||
| * pd.to_numeric(combined.get("regret_weight"), errors="coerce").fillna(0.0) |
There was a problem hiding this comment.
_aggregate_opportunities now assumes columns like regret_weight, kernel_ids, strategy_ids, etc. already exist, but historical opportunity_catalog.csv files (produced before this commit) do not contain them. In that case this line path fails (combined.get("regret_weight") becomes None and later aggregations reference missing columns), so compare_runs crashes when mixing older artifacts with new code instead of producing a study result.
Useful? React with 👍 / 👎.
| else: | ||
| selected = [sorted(calibration_shapes, key=lambda shape: shape.shape_id)[0]] | ||
| selected = [_representative_shape(calibration_shapes)] |
There was a problem hiding this comment.
The default/fallback branch now picks a median “representative” shape, so ProfileSamplingMode.FIRST_CALIBRATION no longer selects the first calibration shape. Since this mode is the default policy, profiling inputs silently shift for many experiments, which changes selector/profile behavior and breaks comparability with prior runs that used the documented first-shape semantics.
Useful? React with 👍 / 👎.
| status = ProfileStatus.INVOCATION_FAILED | ||
| notes = completed.stderr.strip() or completed.stdout.strip() or "ncu invocation failed" | ||
| elif unsupported: | ||
| elif diagnostics.get("matched_row_count", 0) == 0: |
There was a problem hiding this comment.
When _choose_kernel_row returns kernel_attribution_status like regex_ambiguous, matched_row_count can be non-zero while no attributable row exists. This branch only flags NO_PROFILE_DATA when matched_row_count == 0, so ambiguous attribution is misclassified as UNSUPPORTED_COUNTER later, which incorrectly feeds these rows into counter-availability accounting and distorts reportability decisions.
Useful? React with 👍 / 👎.
|
Published the profiling reliability follow-up and the reportability semantics fix to this PR. What changed:
Validation:
I am updating the cluster checkout next and starting that study-eligible ladder. |
1 participant
Summary
This PR fixes the benchmark and analysis issues identified in the review pass so the repo artifacts better match the paper-facing claims.
What changed
Verification
Linked issues
Fixes #2
Fixes #3
Fixes #4
Fixes #5
Fixes #6
Fixes #7
Fixes #8
Fixes #9
Fixes #10