FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: Handle geojson features that cross antimeridian in computeBbox by camdecoster · Pull Request #7891 · plotly/plotly.js · GitHub

/ plotly.js Public

fix: Handle geojson features that cross antimeridian in computeBbox - #7891

Merged
camdecoster merged 10 commits into
v4.0from
cam/4436/handle-geojson-antimeridian
Jul 7, 2026
ConversationCommits (10)ChecksFiles changed
Merged

fix: Handle geojson features that cross antimeridian in computeBbox#7891
camdecoster merged 10 commits into
v4.0from
cam/4436/handle-geojson-antimeridian

Conversation

camdecoster commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Update computeBbox to handle geojson features that cross the antimeridian.

Closes #4436.

Changes

  • Use d3-geo to get coords for features that cross the antimeridian instead of turn/bbox
  • Update computeBbox call sites
  • Add/update tests

Testing

  • Be on master
  • Load Plotly devtools
  • Paste the following snippet into the browser devtools console 
    Plotly.newPlot(gd, [{
    type: 'choropleth',
    locations: ['RUS'],
    z: [1],
    colorscale: [[0, '#c33'], [1, '#c33']],
    showscale: false
}], {
    geo: { fitbounds: 'locations', projection: { type: 'equirectangular' } },
    width: 700,
    height: 500,
    margin: { l: 0, r: 0, t: 0, b: 0 }
});
  • Note that the plot spans the whole world
  • Switch to this branch
  • Run the snippet again
  • Note that the plot spans only Asia

Screenshots

Before After

Notes

  • This has been a long-standing TODO (from 2019)
  • @turf/bbox doesn't handle geometry that spans the antimeridian and there's been no movement to fix that issue
  • This update uses a D3 utility (d3-geo.geoBounds, that does work with the antimeridian) to do the same thing (along with a turf utility used within @turf/bbox)
  • This could be considered a breaking change as some plots will show something different for the same config

camdecoster self-assigned this Jul 6, 2026
camdecoster added this to the v4.0.0 milestone Jul 6, 2026
camdecoster marked this pull request as ready for review July 6, 2026 22:51
emilykl reviewed Jul 7, 2026
View reviewed changes
Comment thread src/traces/choropleth/plot.js Outdated
emilykl reviewed Jul 7, 2026
View reviewed changes
Comment thread src/traces/choropleth/plot.js Outdated
emilykl reviewed Jul 7, 2026
View reviewed changes
Comment thread src/lib/geo_location_utils.js
Comment on lines +396 to +399
* @return {[number, number, number, number]|null} `[west, south, east, north]`
* in degrees; `east` may exceed 180° when the input crosses ±180°.
* Returns `null` for input with no extractable coordinates (e.g. `Sphere`,
* empty FeatureCollection).

emilykl Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you expand this docstring to clarify the numeric range of each coordinate, and any constraints on the output, and also whether east is guaranteed to exceed 180 if the input crosses the antimeridian?

camdecoster Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'll add a bit more info, but some of what you're asking for seems out of scope of a docstring.

emilykl reviewed Jul 7, 2026
View reviewed changes
Comment thread src/lib/geo_location_utils.js Outdated
Comment on lines +415 to +416
// Pass as MultiPoint to sidestep d3-geo's polygon winding assumption (CW
// exterior, opposite of RFC 7946) and geodesic apex overshoot

emilykl Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't know what CW exterior or geodesic apex overshoot mean, could you tweak this comment to be a bit more readable?

camdecoster Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, that's confusing. I'll simplify it.

emilykl reviewed Jul 7, 2026
View reviewed changes
Comment thread src/lib/geo_location_utils.js Outdated
return [
west,
south,
east < west ? east + ANTIMERIDIAN_LON_SHIFT : east, // Unwrap antimeridian crossing; east may exceed 180°

emilykl Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can use unwrapLonRange() here now that #7881 is merged, right?

camdecoster Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This check is slightly different, but I can probably get everything to use the same calc. I'll update it.

emilykl reviewed Jul 7, 2026
View reviewed changes
Comment thread src/lib/geo_location_utils.js Outdated
emilykl reviewed Jul 7, 2026
View reviewed changes
Comment thread src/traces/choropleth/plot.js Outdated
@@ -54,17 +54,25 @@ function calcGeoJSON(calcTrace, fullLayout) {
calcPt._polygons = geoUtils.feature2polygons(feature);

var bboxFeature = geoUtils.computeBbox(feature);

emilykl Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I realize this issue predates your changes, but it seems a little silly to compute bounding boxes for all the features here if they might be ignored in the next step. Maybe a small refactor would make sense?

It's beyond the scope of this PR though so not necessary.

camdecoster Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good idea! That does seem a bit wasteful. Let me poke at it.

emilykl reviewed Jul 7, 2026
View reviewed changes
Comment thread test/jasmine/tests/geo_test.js
}

describe('Test geo fitbounds with antimeridian-straddling points', function() {
describe('Test geo fitbounds with antimeridian-straddling points', function () {

emilykl Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it worth adding a similar test here for choropleth to verify the changes end-to-end? And/or an image test? (I'm a little surprised that none of the existing image baselines changed as a result of these changes, but I guess it requires a pretty specific combination of factors to surface this issue.)

camdecoster Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In the follow up work to default to fitbounds = "locations", I'll address adding tests. This one just needed to be updated because of computeBbox changed.

emilykl approved these changes Jul 7, 2026
View reviewed changes

emilykl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Left some nitpick comments, but LGTM.

camdecoster merged commit 8476f59 into v4.0 Jul 7, 2026
84 checks passed
camdecoster deleted the cam/4436/handle-geojson-antimeridian branch July 7, 2026 22:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

emilykl emilykl approved these changes

Assignees

camdecoster

Labels

None yet

Projects

None yet

Milestone

v4.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL