[BUG] num_scalings is always overcounted by exactly 1 (shift(-1) makes the last row NaN)
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 12
- Forks
- 11
- Avg merge
- 7d 23h
- Merged PRs (30d)
- 3
Description
Bug
calculate_metrics in src/vasim/simulator/analysis/plot_utils.py:104 counts scaling events with a backward shift:
num_changes = (merged["CURR_LIMIT"] != merged["CURR_LIMIT"].shift(-1)).sum()
shift(-1) leaves the last element NaN, and NaN != x is always True in pandas, so the final row is always counted as a scaling event. The result is off by exactly +1 on every run.
Repro
import pandas as pd
d = pd.DataFrame({"CURR_LIMIT": [4, 4, 4, 4, 4]}) # never scales
(d["CURR_LIMIT"] != d["CURR_LIMIT"].shift(-1)).sum()
# 1 <- expected 0
shift(-1): [4.0, 4.0, 4.0, 4.0, nan]
!= gives : [False, False, False, False, True] <- NaN != 4 is always True
Across cases:
| case | CURR_LIMIT | reported | expected |
|---|---|---|---|
| never scales | [4, 4, 4, 4, 4] |
1 | 0 |
| one change | [4, 4, 8, 8, 8] |
2 | 1 |
| two changes | [4, 8, 8, 2, 2] |
3 | 2 |
| single row | [4] |
1 | 0 |
| alternating | [4, 8, 4, 8, 4] |
5 | 4 |
A configuration that never scales at all reports 1 scaling.
Why it matters
num_scalings is not just displayed — it feeds ParetoFrontier normalization (num_scalings / num_scalings.max()) and the Pareto ranking, so tuning comparisons between configurations are skewed by it.
Suggested fix
- num_changes = (merged["CURR_LIMIT"] != merged["CURR_LIMIT"].shift(-1)).sum()
+ num_changes = (merged["CURR_LIMIT"] != merged["CURR_LIMIT"].shift(1)).sum() - 1
Verified this returns 0 / 1 / 2 / 0 / 4 on the cases above.
Companion crash the fix exposes
ParetoFrontier.preprocess_df (src/vasim/simulator/analysis/ParetoFrontier.py:64-69) creates num_scalings_norm only inside a guard, then uses it unconditionally:
if df["num_scalings"].max() > 0:
df["num_scalings_norm"] = df["num_scalings"] / df["num_scalings"].max()
df = df[df["num_scalings_norm"] <= np.percentile(df["num_scalings_norm"], 90)] # KeyError when max == 0
Today num_scalings is never 0 because of the bug above, so this is unreachable. Fixing the off-by-one makes it reachable for any sweep where no configuration scales. The sum_slack / sum_insufficient_cpu guards above it are fine — those columns aren't referenced later — so this is specifically a num_scalings_norm oversight. Worth fixing in the same change:
if df["num_scalings"].max() > 0:
df["num_scalings_norm"] = df["num_scalings"] / df["num_scalings"].max()
-
- df = df[df["num_scalings_norm"] <= np.percentile(df["num_scalings_norm"], 90)]
+ df = df[df["num_scalings_norm"] <= np.percentile(df["num_scalings_norm"], 90)]
return df
Note on existing tests
Four e2e expectations currently hardcode the inflated values and would each need decrementing by 1 alongside the fix:
tests/test_e2e_single_run_sim.py:110(466) and:157(347)tests/test_e2e_multi_run_tune_with_strategy.py:134(109) and:209(133)
Happy to open a PR with the fix plus those four updates if this looks right to you.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in src/vasim/simulator/analysis/plot_utils.py:104 and inspect how calculate_metrics derives num_scalings, then read ParetoFrontier.preprocess_df at src/vasim/simulator/analysis/ParetoFrontier.py:64-69. Update the affected expectations in tests/test_e2e_single_run_sim.py:110,157 and tests/test_e2e_multi_run_tune_with_strategy.py:134,209; done means zero-scale cases work and all four expected values match the corrected counts.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- numpy, pandas, python
- Domain
- data
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100