Skip to content

Heatmap equal gaps #2213

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
Dec 18, 2017
Merged

Heatmap equal gaps #2213

merged 3 commits into from
Dec 18, 2017

Conversation

alexcjohnson
Copy link
Collaborator

Fixes #2142 - heatmaps with more than 3 bricks in one direction had nonuniform gaps. Looking into it in more detail, the bricks at the edges were also not centered on their desired data values, as the gap was applied only to the inside edges of these gaps.

This PR makes all the gaps symmetric (aside from rounding to keep the brick edges as crisp as possible).

Note the two commits here:

  • 2802c79 just changes the mock & baseline image to make the issue obvious
  • 0d1709b changes the algorithm and updates the baseline image and jasmine tests - please see the image diff in that commit to get a feel for what the new algorithm does differently. In particular, the bricks are all now a bit smaller than they were before, since some of the gaps had to expand to be as big as requested, as well as adding gaps around the edges to keep the bricks centered.

cc @CalvinFernandez @etpinard

hopefully avoid this particular spurious test failure...
})
.catch(failTest)
.then(done);
}, LONG_TIMEOUT_INTERVAL);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We've seen this one fail a lot - and this morning seems like circle is super stressed or something, every intermittent test failure we've ever had seems to be biting us today. Choropleth needs to make a request for map data - it's local but still perhaps slow? Just give it some more time before we fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's local but still perhaps slow?

Actually, no. Geo subplots in jasmine tests get their map data from https://cdn.plot.ly/world_110m.json and friends.

You brought this up previously (see #163) but I never got around to fixing that.


👌 for bumping the timeout interval.

@etpinard
Copy link
Contributor

etpinard commented Dec 18, 2017

Excellent fix & tests 💃

@alexcjohnson alexcjohnson merged commit 521bd55 into master Dec 18, 2017
@alexcjohnson alexcjohnson deleted the heatmap-equal-gaps branch December 18, 2017 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants