Skip to content

expose scales #538

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 87 commits into from
Sep 26, 2021
Merged

expose scales #538

merged 87 commits into from
Sep 26, 2021

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Sep 10, 2021

exposing scales, extracted from #484

build at https://observablehq.com/@observablehq/expose-plot-scales-538

With many tests!

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of materializing a family field on the internal scale object, I wonder if we can have a function that takes a specific scale type and returns either a corresponding family, or a boolean. For example:

function isTemporalType(type) {
  return type === "time" || type === "utc";
}

Or the other approach:

const quantitative = Symbol("quantitative");
const temporal = Symbol("temporal");
const ordinal = Symbol("ordinal");

function family(type, key) {
  switch (type) {
    case "diverging":
    case "diverging-sqrt":
    case "diverging-pow":
    case "diverging-log":
    case "diverging-symlog":
    case "cyclical":
    case "sequential":
    case "linear":
    case "sqrt":
    case "threshold":
    case "quantile":
    case "pow":
    case "log":
    case "symlog":
      return quantitative;
    case "identity":
      return registry.get(key) === position ? quantitative : ordinal;
    case "utc":
    case "time":
      return temporal;
    case "ordinal":
    case "categorical":
    case "point":
    case "band":
      return ordinal;
  }
}

This would be useful when coercing channel values based on the scale type, for example. (I haven’t reviewed the suggested code above closely…)

@Fil Fil marked this pull request as ready for review September 15, 2021 10:33
@Fil Fil requested a review from mbostock September 16, 2021 11:20
@mbostock
Copy link
Member

I’m going to start working on this. I rebased against main and I got a few test failures. I skipped one and regenerated the polylinear test. Is this the expected output?

Screen Shot 2021-09-24 at 5 45 23 PM

@Fil
Copy link
Contributor Author

Fil commented Sep 25, 2021

All tests are fixed now. For reference the correct polylinear test is:
Capture d’écran 2021-09-25 à 08 18 36

(The error was in maybePiecewiseRange which was checking the old type==quantitative.)

@Fil Fil mentioned this pull request Sep 25, 2021
@mbostock
Copy link
Member

Thanks for the help. I’m going to continue rewriting the tests today (schedule permitting).

@mbostock
Copy link
Member

mbostock commented Sep 25, 2021

Okay, I think this is 99% ready. The last things I want to think through are:

  1. The semantics of the isDescendingDomain test versus the reverse options. Thank you for replying to my notebook! Probably either semantics are fine, I just want to feel comfortable with it before committing.
  2. The internal confusion around scale options vs. axis options for label and labelAnchor, and specifically whether we should also materialize axis-related options in the returned scale object.

The latter one embodies the difference between Plot’s internal and external representations. Externally, scales and axes options are on the same object, but internally, we split them up. Probably we want to favor the external representation when we materialize scale objects and return them from plot.scale. Although I suppose alternatively we could add a plot.axis method in the future, and you’d be expected to splat them together if needed. But at any rate since we are already exposing the label axis option on the returned scale object, I’m guessing that we do in fact want to include all the axis options in the exposed scale and use a unified representation.

Okay, maybe that’s more like 92% ready. 😉

Fil and others added 11 commits September 25, 2021 16:46
Co-authored-by: Mike Bostock <[email protected]>
Co-authored-by: Mike Bostock <[email protected]>
Co-authored-by: Mike Bostock <[email protected]>
Co-authored-by: Mike Bostock <[email protected]>
tiny difference with the parent commit: a non-position scale (such as opacity) can have an automatic label (including "%")
@mbostock mbostock merged commit 5d399c6 into main Sep 26, 2021
@mbostock mbostock deleted the fil/expose-scales-2 branch September 26, 2021 00:32
@mbostock mbostock mentioned this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants