microsoft / microsoft/vasim

[BUG] num_scalings is always overcounted by exactly 1 (shift(-1) makes the last row NaN)

Open
#136 0 comments 0 reactions 0 assignees View on GitHub

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.

https://claude.ai/code/session_01CFRv5kvesfnKYgHmNw9Vpf

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.