Skip to content

follow-up: derive x & y scale domains from geometry #1663

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 3 commits into from
Jun 2, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jun 1, 2023

addresses TODO items in #1468 and adds/tweaks tests

@Fil Fil requested a review from mbostock June 1, 2023 08:25
src/plot.js Outdated
const [x, y] = getGeometryChannels(channel);
addScaleChannel(channelsByScale, "x", x);
addScaleChannel(channelsByScale, "y", y);
if (projection == null && x?.domain === undefined && y?.domain === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we’ll need a fancier test than projection == null, per this comment on projectionAspectRatio:

plot/src/projection.js

Lines 225 to 241 in df3a3ad

// When a named projection is specified, we can use its natural aspect ratio to
// determine a good value for the projection’s height based on the desired
// width. When we don’t have a way to know, the golden ratio is our best guess.
// Due to a circular dependency (we need to know the height before we can
// construct the projection), we have to test the raw projection option rather
// than the materialized projection; therefore we must be extremely careful that
// the logic of this function exactly matches Projection above!
export function projectionAspectRatio(projection, marks) {
if (typeof projection?.stream === "function") return defaultAspectRatio;
if (isObject(projection)) projection = projection.type;
if (projection == null) return hasGeometry(marks) ? defaultAspectRatio : undefined;
if (typeof projection !== "function") {
const {aspectRatio} = namedProjection(projection);
if (aspectRatio) return aspectRatio;
}
return defaultAspectRatio;
}

A quick take, but should have more thought:

function hasProjection({projection} = {}) {
  if (projection == null) return false;
  if (typeof projection.stream === "function") return true;
  if (isObject(projection)) projection = projection.type;
  return projection != null;
}

But the bad part is that the projection can be specified as a function (a “projection initializer”) which can return null indicating that there isn’t actually a projection. But maybe we don’t care about that nuance here, since this is really just a performance optimization and shouldn’t change the behavior?

Copy link
Contributor Author

@Fil Fil Jun 1, 2023

Choose a reason for hiding this comment

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

I don't think we even need to cover the projection: null and projection: {type: null} cases, tbh. Having a smart domain when projection===undefined would be enough to solve the casual use of Plot.geo with some geojson. If the user starts to specify a projection, then they know what they're doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm it was the other way round: we can only skip that geometry look-up if the projection is guaranteed to be non-nullish. Which is what you were saying :)

@mbostock mbostock force-pushed the mbostock/geo-scales branch from 0dea987 to a6d4533 Compare June 2, 2023 15:50
@mbostock mbostock merged commit eca4083 into mbostock/geo-scales Jun 2, 2023
@mbostock mbostock deleted the fil/geo-scales branch June 2, 2023 15:55
mbostock added a commit that referenced this pull request Jun 2, 2023
* derive x & y scale domains from geometry

* follow-up: derive x & y scale domains from geometry (#1663)

* fix pending issues and add tests

* optimize and comment

* move hasProjection

---------

Co-authored-by: Mike Bostock <mbostock@gmail.com>

---------

Co-authored-by: Philippe Rivière <fil@rezo.net>
Fil added a commit that referenced this pull request Aug 21, 2023
* derive x & y scale domains from geometry

* follow-up: derive x & y scale domains from geometry (#1663)

* fix pending issues and add tests

* optimize and comment

* move hasProjection

---------

Co-authored-by: Mike Bostock <mbostock@gmail.com>

---------

Co-authored-by: Philippe Rivière <fil@rezo.net>
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* derive x & y scale domains from geometry

* follow-up: derive x & y scale domains from geometry (observablehq#1663)

* fix pending issues and add tests

* optimize and comment

* move hasProjection

---------

Co-authored-by: Mike Bostock <mbostock@gmail.com>

---------

Co-authored-by: Philippe Rivière <fil@rezo.net>
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