auto margins #1722

Fil · 2023-06-25T16:12:40Z

Alternative to #1714. The axis mark sets the value of marginLeft (etc.) not to a number, but to a special value ("auto", not a symbol if we want to keep this usable from outside); the actual numeric value of that special value is computed in createDimensions—where the scales’ types and domains are already materialized

TODO:

facet axes?
find a “good” heuristic (consider the scales’ domain, ticks, tickFormat…)
consider labelAnchor ("center" needs more space)
use approx. text metrics instead of string.length
account for the possibility of a centered axis label
fit & test monospace (see plot().dimensions() returns the chart's dimensions #1877 for easier tests?)

closes #1451
closes #383
closes #1859

Fil · 2023-09-14T15:22:59Z

This feels almost ready; needs a bit of manual testing (and more snapshot tests to capture more ranges of ticks?). In particular with the auto mark.

cc @tophtucker

mbostock · 2023-09-14T15:42:27Z

Can you regenerate the snapshots so we can more easily review the impact of this change? Thank you. 🙏

Fil · 2023-09-14T15:45:44Z

Here's a playground to develop tests: https://observablehq.com/@observablehq/auto-margins-1722

test/output/beckerBarley.svg

test/output/usPopulationStateAgeGrouped.svg

test/output/usPresidentialElection2020.svg

mbostock

I think we’ll need to account for the tickFormat option when computing the automatic margins (even if it’s a custom function). It should be possible to compute the formatted tick strings, and then use their text metrics to more accurately infer the needed margins? Rather than assuming that numbers/dates/etc. are formatted a specific way?

Fil · 2023-09-14T16:29:05Z

Computing all the ticks is probably more adequate, but it is also quite a bit more complicated, since the ticks are computed from the instantiated scales. I've tried to approximate it by doing half-way measures (like, apply tickFormat if given… but on what ticks?) and it seemed worse. My conclusion was that either we have a light system (this one, to which we could add a few common cases such as type: "log"), or we go full-in and measure the actual text that will be rendered, by running a (fake) axisY mark with all the options?

mbostock · 2023-09-14T16:42:53Z

Yes, it’s complicated. 🤔

mbostock · 2023-09-20T17:38:50Z

As discussed, we want to initialize (but not fully render) the axis text mark for y scales; this will give us the text channel which we can use to infer appropriate left & right margins.

src/dimensions.js

mbostock

If we support auto for marginLeft and marginRight, we should also support it for marginTop and marginBottom (even if those margins are comparatively “dumb” and just have the current behavior).

mbostock · 2023-09-20T17:41:38Z

src/mark.js

    this.marginBottom = +marginBottom;
-    this.marginLeft = +marginLeft;
+    this.marginLeft = marginLeft === "auto" ? "auto" : +marginLeft;


Needs isAuto helper (for case-insensitivity).

test/output/usStatePopulationChangeRelative.svg

Fil · 2023-09-22T17:05:25Z

I've implemented the new strategy. Still needs a bit of work:

~~clean up the code~~
~~fix the two test cases mentioned above~~
~~keyword check on "auto"~~ (no more keywords!)
~~consider auto for top and bottom margins~~

test/output/penguinMassSexSpecies.svg

test/output/penguinFacetDodgeSymbol.html

test/output/penguinDodgeHexbin.svg

Fil · 2023-09-27T15:33:52Z

I think this is good to go! Does it need documentation?

mbostock · 2023-09-27T17:43:52Z

src/dimensions.js

+// scales a second time.
+function autoMarginK(margin, {k: scale, labelAnchor, label}, options, mark, stateByMark, scales, dimensions, context) {
+  const {data, facets} = stateByMark.get(mark);
+  const {channels} = mark.initializer(data, facets, {}, scales, dimensions, context);


Don’t you want to call mark.initialize here, not mark.initializer? The latter only applies to marks with an initializer, but here we want to initialize the mark (without assuming that it has an initializer).

I think we need the initializer because the actual ticks depend on the instantiated scales. We know that the mark has an initializer because it can only be a (tick label) axis mark.

My point is that we should follow the same code path that Plot.plot follows:

call mark.initialize first

if mark.initializer exists, call that second

We shouldn’t make such a strong assumption about how the mark is implemented. Also, I wasn’t expecting that the mark would already be initialized (using stateByMark); I was thinking we would initialize it ourselves here. So that further complicates matters.

When we call autoMarginK all the marks have been initialized already, which is why we can avoid the cost of re-initializing them. I can see how reaching for stateByMark can feel messy, though. 🥺

If the mark is already initialized here, why do we need to invoke its initializer again?

mbostock · 2023-09-27T17:46:21Z

src/marks/axis.js

@@ -81,9 +81,9 @@ function axisKy(
    x,
    margin,
    marginTop = margin === undefined ? 20 : margin,
-    marginRight = margin === undefined ? (anchor === "right" ? 40 : 0) : margin,
+    marginRight = margin === undefined ? (anchor === "right" ? (x == null ? k : 40) : 0) : margin,


Temporarily setting this to k feels a bit dirty… it’s effectively a hidden API if you set the marginRight option to "x" or "y", right?

Agree, but I didn't find a better way.

You’ll likely have to change how the defaults are initialized rather than using default initializers.

mbostock · 2023-09-27T17:49:03Z

src/plot.js

+  const {scaleDescriptors, scales, dimensions, subdimensions, superdimensions} = createDimensionsScales(
+    channels,
+    marks,
+    stateByMark,
+    options,
+    context
+  );


We might want to re-inline the createDimensionsScales function into plot here, given the number of arguments and return values — not sure it’s beneficial to break it out into a separate function. Perhaps I should investigate and see how it feels.

The thing is we need to run it twice, so inlining it means we have to repeat these lines. Also it felt better to have the part about autoMargins live in dimensions.js (although I'd prefer to have the scales part live in plot.js… and they're intertwined!

I've inlined and minimized it. Hope this is more readable

mbostock

This looks great overall. I think I want to fiddle with the implementation a bit, and in particular I want to use mark.initialize instead of mark.initializer, but the results seem good and the implementation manageable. Do you feel good about it?

Fil · 2023-09-27T21:36:36Z

Yes I feel good about the result: the tests are all looking great and the only changes are improvements. I think the algorithm is correct. The implementation can certainly be better.

Fil · 2023-09-27T21:39:18Z

One thing that bugged me is that monospaceWidth and defaultWidth are not aligned; maybe there's still time to change this by making monospace chars 60 instead of 100 — it would be a slightly breaking change, but it would make the two lengths comparable, which is something that was needed here.

alternative to #1714 closes #1451

…of the actual tick labels is too large.

- note that the widths are not comparable! - needs tests

fix a bug with inferred labels, which can be empty ("Date", "Year"… are filtered out). observe marginLeft and marginRight when specified in the axis mark (this wasn't tested and had lead to a regression)

(tested by http://localhost:8008/?test=marginsMonospace)

Fil requested a review from mbostock June 25, 2023 16:12

Fil mentioned this pull request Jun 25, 2023

60px default y-axis margin #1714

Closed

Fil mentioned this pull request Aug 21, 2023

Tracking issue for 1.0 #1711

Open

6 tasks

Fil marked this pull request as ready for review September 14, 2023 15:23

mbostock reviewed Sep 14, 2023

View reviewed changes

test/output/beckerBarley.svg Outdated Show resolved Hide resolved

mbostock reviewed Sep 14, 2023

View reviewed changes

test/output/usPopulationStateAgeGrouped.svg Outdated Show resolved Hide resolved

mbostock reviewed Sep 14, 2023

View reviewed changes

test/output/usPresidentialElection2020.svg Outdated Show resolved Hide resolved

mbostock reviewed Sep 14, 2023

View reviewed changes

mbostock reviewed Sep 20, 2023

View reviewed changes

src/dimensions.js Outdated Show resolved Hide resolved

mbostock reviewed Sep 20, 2023

View reviewed changes

Fil commented Sep 22, 2023

View reviewed changes

test/output/usStatePopulationChangeRelative.svg Outdated Show resolved Hide resolved

Fil commented Sep 22, 2023

View reviewed changes

test/output/penguinMassSexSpecies.svg Outdated Show resolved Hide resolved

Fil commented Sep 22, 2023

View reviewed changes

test/output/penguinFacetDodgeSymbol.html Outdated Show resolved Hide resolved

Fil commented Sep 22, 2023

View reviewed changes

test/output/penguinDodgeHexbin.svg Outdated Show resolved Hide resolved

Fil mentioned this pull request Sep 26, 2023

plot().dimensions() returns the chart's dimensions #1877

Draft

mbostock reviewed Sep 27, 2023

View reviewed changes

Fil added 27 commits July 29, 2024 08:09

auto margins

1debd94

alternative to #1714 closes #1451

progress save

a4ebdee

fy (and flipped y/fy)

2c50736

impact on tests

79fa805

autoMarginK signature

d7916fc

When margins are "auto", run the dimensions calculation twice if any …

cd88d7f

…of the actual tick labels is too large.

fix tests

39e2852

changed plots

353f045

295

a7f86ae

we need to know if it's y or fy, to apply a margin or facet margin

9cc55bd

fix for when the axis has x positioning

d8559ab

refactor

df517c3

no special keywords

277734e

coerce tick label to string before calling defaultWidth

6e48b06

monospace

4dc5624

- note that the widths are not comparable! - needs tests

simpler (we'll see later if we need to change the values for monospace)

de4251e

skeleton of tests

76a23be

OK!

8b29b78

fix a bug with aspectRatio

bacd5b9

don't suppose that the mark that pilots autoMargin has an initializer

350c182

add test for aspectRatio and margins

165056d

nicer defaults

015711f

typo

63070d9

inline & clean up

a8ee323

auto-margin top and bottom (closes #1859)

64538a8

fix a bug with inferred labels, which can be empty ("Date", "Year"… are filtered out). observe marginLeft and marginRight when specified in the axis mark (this wasn't tested and had lead to a regression)

adopts the new monospace relative width (#1880)

4bec5b5

(tested by http://localhost:8008/?test=marginsMonospace)

fix tests

cb3d714

Fil force-pushed the fil/default-margins branch from 0b9889a to cb3d714 Compare July 29, 2024 12:49

Fil added 2 commits August 6, 2024 14:16

Merge branch 'main' into fil/default-margins

bb905bd

Merge branch 'main' into fil/default-margins

1f01c40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auto margins #1722

auto margins #1722

Fil commented Jun 25, 2023 •

edited

Loading

Fil commented Sep 14, 2023

mbostock commented Sep 14, 2023

Fil commented Sep 14, 2023

mbostock left a comment •

edited

Loading

Fil commented Sep 14, 2023

mbostock commented Sep 14, 2023

mbostock commented Sep 20, 2023

mbostock left a comment

mbostock Sep 20, 2023

Fil commented Sep 22, 2023 •

edited

Loading

Fil commented Sep 27, 2023

mbostock Sep 27, 2023

Fil Sep 27, 2023

mbostock Sep 27, 2023

Fil Sep 27, 2023

mbostock Sep 28, 2023

mbostock Sep 27, 2023

Fil Sep 27, 2023

mbostock Sep 27, 2023

mbostock Sep 27, 2023

Fil Sep 27, 2023

Fil Sep 28, 2023

mbostock left a comment

Fil commented Sep 27, 2023

Fil commented Sep 27, 2023 •

edited

Loading

auto margins #1722

Are you sure you want to change the base?

auto margins #1722

Conversation

Fil commented Jun 25, 2023 • edited Loading

Fil commented Sep 14, 2023

mbostock commented Sep 14, 2023

Fil commented Sep 14, 2023

mbostock left a comment • edited Loading

Choose a reason for hiding this comment

Fil commented Sep 14, 2023

mbostock commented Sep 14, 2023

mbostock commented Sep 20, 2023

mbostock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fil commented Sep 22, 2023 • edited Loading

Fil commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbostock left a comment

Choose a reason for hiding this comment

Fil commented Sep 27, 2023

Fil commented Sep 27, 2023 • edited Loading

Fil commented Jun 25, 2023 •

edited

Loading

mbostock left a comment •

edited

Loading

Fil commented Sep 22, 2023 •

edited

Loading

Fil commented Sep 27, 2023 •

edited

Loading