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

Adds Heatmap and allows arbitrary number of threshold colors #35

Closed
wants to merge 8 commits into from
Closed

Adds Heatmap and allows arbitrary number of threshold colors #35

wants to merge 8 commits into from

Conversation

samhatchett
Copy link
Contributor

addresses #25 and #17

  • color-pickers automatically appear/vanish as user enters/removes threshold values
  • optionally plots values using Leaflet.heat - user sets colors for gradient and blur/radius
  • minor adjustments to caching of circles to improve lookup time (helps for large data sets)

@samhatchett
Copy link
Contributor Author

@daniellee mentioned that PRs are welcome for these issues. I did not include dist files in this PR - my system seems to create very differently formatted files, so let me know if that is needed and I'll investigate.

@daniellee
Copy link
Contributor

Thank you @samhatchett I'll check it out and try and get it merged in this week.

@apkoponen
Copy link

This looks exactly what we've been searching.

@daniellee
Copy link
Contributor

Finally got around to checking this out. Looks very nice. There are a few issues which are my fault due to not having documented the build process and not having set up circle ci yet (a bunch of tests are red).

I think we should add the license for leaflet.heat as well.

Really liked the dynamic thresholds fix. It seems to work really well. Once I've got the tests fixed and some documentation written, I will merge and release this.

@samhatchett
Copy link
Contributor Author

Good to hear. Please let me know how I can be helpful!

@daniellee
Copy link
Contributor

@samhatchett I spent some time on this over the last few days and I don't think the heatmap plugin works properly. I found this issue: Leaflet/Leaflet.heat#43 where you had commented on it. I tried with the version of leaflet.heat in this PR and the latest version from the leaflet.heat repo.

I tested with some fake data. I have 5 countries, 2 have of them have a value of 0. 1 has value of just over 1500 and the other 2 have values over 1600.

This is what it looks like with circles:
image

With the heatmap layer, you can barely see the colors:
image

I would be expecting the intensity to be much brighter. I debugged the code and your code looks fine, the options seem to be correct.

If remove the options with the gradients/thresholds and the max value, then the intensity looks more like I expect:

image

Any thoughts? Am I thinking about this the wrong way or is this a bug?

@samhatchett
Copy link
Contributor Author

@daniellee I think you're right on target. The thresholds are not really tied directly to point values, and the zoom level greatly influences the appearance. It seems to me that Leaflet.heat was intended to be used in cases of huge numbers of points (since it has builtin subsampling), as a way to show qualitative point density. I'm trying to do two things that are divergent from that - show quantitative thresholds, and show values of locations rather than density of points.

I have been investigating other alternatives since L.heat doesn't really solve the problem in the right way for me (and probably most WorldMap users?) The alternative helpfully suggested in that L.heat issue thread looks promising, with better options about scaling radii and using local/global maxima for coloration. I've also worked some with rendering gradients within Delaunay triangles (image below) or coloring Voronoi cells as a way to generate a more polygonal heat map
screen shot 2016-09-29 at 10 07 52 am

I'm planning to spend some more time on this in the coming days - if you have any thoughts on direction or the implementation of what you've seen so far please let me know. Also, i'm happy to close this PR for now since there's still some real work to do, or leave it parked here in case others are interested in commenting.

@daniellee
Copy link
Contributor

Going to cherry pick the thresholds change from this PR and release it this week. Thanks @samhatchett for that!

@samhatchett
Copy link
Contributor Author

Cool, thanks! I should have staged them separately, just didn't anticipate the additional complexity on the heat map issue.

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.

3 participants