Skip to content
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

Geo grid fixes #3706

Merged
merged 9 commits into from Apr 5, 2019

Conversation

Projects
None yet
2 participants
@etpinard
Copy link
Member

commented Apr 1, 2019

fixes #2896 (finally) making geo.(lon|lat)axis.tick0 work by computing grid values using Axes.calcTicks.

cc @plotly/plotly_js - I'd like to slip this one in 1.46.0.

@archmoj

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

Great!
Concerning the baselines changes,
@etpinard could you please elaborate why longitudes are changed on polar stereographic projection:
https://github.com/plotly/plotly.js/pull/3706/files?short_path=2b60c83#diff-2b60c83395ec0c848268b4c720e7d73b
Thanks.

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

could you please elaborate why longitudes are changed on polar stereographic projection:

We have a slightly different tick algo than d3.geo.graticule.

@archmoj

This comment has been minimized.

Copy link
Collaborator

commented Apr 1, 2019

I think the new longitude lines displayed on the polar stereographic projection may be off by one/half pixel or a distance.
For example 20N should cross Hilo island https://www.mapsofworld.com/usa/states/hawaii/lat-long.html.

Old:
old
New:
new

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

Sure, there's a diff. Now, is the old baseline correct, or is the new baseline better?

Oh well, I guess I hold this one out of 1.46.0

@etpinard etpinard added this to the v1.47.0 milestone Apr 1, 2019

@archmoj

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2019

Sure, there's a diff. Now, is the old baseline correct, or is the new baseline better?

Hard to tell. To my eyes the old baseline appear to be slightly more accurate if we compare against Hawaii.

etpinard added some commits Apr 3, 2019

make geo.(lon|lat)axis.tick0 dflt=0
- so that it matches the current behavior (i.e. where
  tick0 didn't do a thing)
@etpinard

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@archmoj please have a look at fdfca3b where by changing the tick0 dflt, we retrieve something almost identical to the baselines on master.

@archmoj

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

Excellent.
@etpinard could you please increase the width and the height of this mock: test/image/baselines/geo_tick0.png

@archmoj

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

Looks great!
💃 when you wanted to start 1.47.0.

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

when you wanted to start 1.47.0.

I'll wait for Monday.

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Ok, no new bug reports from 1.46.0 today, so let's start merging stuff for next week's 1.47.0

@etpinard etpinard merged commit 0b4e549 into master Apr 5, 2019

10 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish Your tests passed on CircleCI!
Details
ci/circleci: test-bundle Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine3 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details

@etpinard etpinard deleted the geo-grid-fixes branch Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.