fix axis offset in snapshots

[Fil]

Context:

The diff for #2373 was hard to read initially because a lot of changes stem from the fact that the axis offsets are removed.

To get deep in the woods: our snapshots in main, still using mocha, are in fact inconsistent. The D3 axes have an offset of 0 (window is not defined when we load the module), whereas other elements in Plot use an offset of 0.5 (the fake global.window has been defined).

This branch adds devicePixelRatio = 2 to global.window, and hard-codes offset = 0 so that the snapshots are now consistently using an offset 0 both in d3-axis and in Plot 😅

Lots of diff, but easy to grep … the only changes (ideally) are in the Plot axes translate property, for instance:

-      <g class="tick" opacity="1" transform="translate(0.5,0)">
+      <g class="tick" opacity="1" transform="translate(0,0)">

However! when we actually regenerate the snapshots, we also get the updated "3 digits after decimal dot" precision change which we had never pushed, so we'll have to put a bit more effort into verifying the diff.

And also! By changing the offset, we modify some scales, and the hexbin/hexgrid coordinates change too, resulting in sometimes strikingly different images. So we have to fix that issue (which in my book is a bug: the fact that we render on a low DPI screen should not change the binning), and make hexbin independent of the offset.

So overall the changes to the snapshots fall into several categories. It's a lot to review manually, but a script can make this easier:

Details

// Usage: node verify.js

import {spawn} from "node:child_process";
import {max} from "d3";

function git(...args) {
  return new Promise((resolve, reject) => {
    const chunks = [];
    const child = spawn("git", args);
    child.stdout.on("data", (chunk) => chunks.push(chunk));
    child.on("close", (code) => code === 0 ? resolve(Buffer.concat(chunks).toString()) : reject(new Error(`git ${args[0]} failed`)));
    child.on("error", reject);
  });
}

const from = "origin/main";
const to = "origin/fil/fix-axis-offset";
const [diffOut, treeOut] = await Promise.all([
  git("diff", `${from}..${to}`, "--name-only", "--", "test/output/"),
  git("ls-tree", "-r", "--name-only", from, "test/output/")
]);
const files = diffOut.trim().split("\n").filter(Boolean);
const allFiles = treeOut.trim().split("\n").filter(Boolean);

const numRe = /-?\d+(\.\d+)?/g;

function removeTranslates(s) {
  return s.replace(/ transform="translate\([^)]*\)"/g, "");
}

// Classify numbers after stripping translates.
function classifyNumbers(old, neu) {
  const oldNums = Array.from(old.matchAll(numRe), (m) => +m[0]);
  const newNums = Array.from(neu.matchAll(numRe), (m) => +m[0]);
  if (oldNums.length !== newNums.length) return "needs-review";
  const maxDiff = max(oldNums, (a, i) => Math.abs(a - newNums[i]));
  if (maxDiff === 0) return "translate-only";
  if (maxDiff < 0.001) return "precision-only";
  if (maxDiff < 0.501) return "offset-shift";
  if (maxDiff < 1.001) return "integer-rounding";
  return "needs-review";
}

// Classify each file and collect into categories.
const categories = new Map([
  ["unchanged", []],
  ["translate-only", []],
  ["precision-only", []],
  ["offset-shift", []],
  ["integer-rounding", []],
  ["needs-review", []]
]);

const changedSet = new Set(files);

for (const file of allFiles) {
  if (!changedSet.has(file)) {
    categories.get("unchanged").push(file);
  }
}

await Promise.all(files.map(async (file) => {
  const [old, neu] = await Promise.all([git("show", `${from}:${file}`), git("show", `${to}:${file}`)]);
  const category = classifyNumbers(removeTranslates(old), removeTranslates(neu));
  categories.get(category).push(file);
}));

for (const [key, items] of categories) {
  console.log(`\n${key}: ${items.length} files`);
  for (const f of items) console.log(`  ${f}`);
}

Results:

unchanged: 27 files
  ...
translate-only: 258 files
  ...
precision-only: 290 files
  ...
offset-shift: 206 files
  ...
integer-rounding: 6 files
  ...
needs-review: 11 files
  test/output/penguinVoronoi1D.svg
  test/output/faithfulDensity1d.svg
  test/output/geoTipXY.svg
  test/output/geoTipGeoCentroid.svg
  test/output/geoTipCentroid.svg
  test/output/geoTip.svg
  test/output/projectionBleedEdges2.svg
  test/output/projectionFitBertin1953.svg
  test/output/walmarts.html
  test/output/walmartsDensity.svg
  test/output/walmartsAdditions.svg

[Fil]

keeping as a draft since it's just a stepping stone for #2373

[mbostock]

This breaks a feature of Plot, though? This offset is used to have crisp rendering for lo-dpi screens. I’m okay with fixing the behavior in tests, but it seems wrong to remove this feature from production usage of Plot.

[Fil]

Yes, but I'm doing it here only to get the tests right — the offset is restored in #2373, which is the "second part" of this PR (one to fix the snapshots, the other to swap the test framework).

(I could have spent more time to try and figure out how to fix the tests in mocha, but that would only have been to drop them immediately after.)

[mbostock]

We shouldn’t need to change the behavior in production in order to get the tests right; we can target the test environment. We can’t land this PR if it removes support for lo-dpi screens.

[Fil]

I thought it might be complicated but in fact I can just use the new logic (which defaults offset to 0 when there is no window), to prepare the new snapshots.

And I've also applied your suggestion (as below), as a fly-by "consistency fix".

[mbostock]

Your description says the “D3 axes have an offset of 0 (window is not defined when we load the module)”, but as I understand it, we only use d3-axis in exactly one place: axisBottom in the ramp legend. Everywhere else we’ve implemented ourselves (in the axis mark). Is that the one place you are trying to fix, to be consistent? Or are there other places that I overlooked?

We can (and should) use axis.offset to ensure that any use of d3-axis explicitly uses the same offset we use internally in Plot.

[mbostock]

I see several decomposable pieces here:

1. Have the tests explicitly call setOffset rather than having the offset definition check if it’s in a test environment (using typeof it === "undefined"). This makes it easier for us to change the test environment (i.e., to adopt Vitest) because we more explicitly control the offset used during testing.
2. Fix the inconsistent axis.offset in the ramp legend.
3. Change the tests to use an offset of 0 instead of 0.5.
4. Regenerate the snapshots to use reduced precision.
5. Change the offset behavior of the hexgrid mark and transform.

I’ve implemented (1) and (2) in #2376 which is a tiny +10 -2 PR.

I’m torn on whether we should do (3). On the one hand, this would mean we are testing the common case rather than the edge case. But the no-offset case is simpler, and I’d still want to have some tests somewhere for the offset case. So I’m tempted to continue testing the 0.5 offset even if it’s the uncommon case… ideally we have both? In theory that should be possible using the new setOffset, at least assuming we call it immediately prior to rendering.

We should do (4) independently of any code changes, since it will cause a massive diff.

We should do (5) independently of any (other) snapshot changes, so that we can review what changed.

[mbostock]

I’ve now broken (5) out into #2377 so we can review the isolated changes.