Skip plotting benchmarks without data#9085
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
important: Walkthrough
ChangesData Validation and Skipping
Assessment against linked issues
Out-of-scope changes
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ea645a47-557f-4498-8d12-c8cfb4ef03fe
📒 Files selected for processing (1)
benchmarks/scripts/sol.py
| fused_algname = algname.replace("bench.", "") + "." + subbench | ||
| if df.empty: | ||
| print( | ||
| f"WARNING: Skipped {fused_algname} because no data is present" | ||
| ) | ||
| print(df) | ||
| continue | ||
| if df["bw"].dropna().empty: | ||
| print( | ||
| f"WARNING: Skipped {fused_algname} because it does not report bandwidth" | ||
| ) | ||
| continue |
There was a problem hiding this comment.
important: Guard for missing bw column before indexing at Line 70. Right now df["bw"] can still raise KeyError if the benchmark output omits bw entirely (not just NaN values), which defeats the hardening in this PR. Use a schema check in the skip condition, e.g. treat ("bw" not in df.columns) the same as missing bandwidth data.
There was a problem hiding this comment.
I think a KeyError is appropriate, since then the input is wrong.
There was a problem hiding this comment.
@bernhardmgruber Understood — the five affected benchmarks have bw present but all-NaN, so dropna().empty is the right check for the actual issue. A fully absent bw column is structurally malformed input and a KeyError is a reasonable loud failure there. Fair enough.
There was a problem hiding this comment.
Important: alg_dfs() can now return an empty dict when all selected benchmarks are skipped, which causes alg_bws() to crash because medians remains None. We should add an early return with an error or warning message
Fixes: #6729