-
Notifications
You must be signed in to change notification settings - Fork 16
fix: DH-21259: Fix maps and add docs #1279
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
base: main
Are you sure you want to change the base?
Conversation
mofojed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor things to fix up, but some other thoughts/comments:
- There's a lot of options here we're not showing (projection, map_style, zoom, center, etc). Not saying we need to show all of them, but one example showing a few of those other options would be great to show off the variety of things we can do. The options are already documented which is nice.
- Maybe we should make a data generator like we have for stocks/fish_market to generate an interesting data set. Doing a wave with the lat/lon is okay, but it's not interesting. Having a data generator that generates something a little more interesting or having a couple options would be nice; e.g. even something that's just focused on one country (e.g. Canada 🍁 ) would be interesting and may drive home some of the differences in the examples. Like right now all the plots kind of start off looking the same, and even between
line_mapandline_geo, it's not clear the level of detail that is different on these maps since we're zoomed all the way out.
| ) | ||
|
|
||
| # Create the density map plot | ||
| # Color is set for better visibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Colour isn't being set here, this comment is confusing.
| # Color is set for better visibility |
| ) | ||
|
|
||
| # Create the density map plot | ||
| # Color is set for better visibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Color is set for better visibility |
| # Color is set for better visibility | ||
| density_map_plot = dx.density_map(path, lat="Lat", lon="Lon", z="Z") | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also have an example where the lats/lons are spaced apart a bit more? Right now it just kind of looks like a trailing blob... unsure... I was going to look at the plotly express examples to see what they have, but the link is broken 🤔 : https://plotly.com/python/tile-density-heatmaps/
| if center is None: | ||
| # Set a default center to prevent px from auto-centering based on data | ||
| # which breaks the initial view for map plots since the data may be null initially | ||
| # Auto centering on the server side is also a bad idea since the data can change, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I could us wanting to auto-centre based on the data, when it ticks again... would that be possible? Any way to force Plotly to re-centre when we get a new tick if no centre is specified? I think that might be better than this... either way can file as a separate ticket.
Adds basic docs for map type charts.
Contains a fix for
center, which was broken by default forscatter_map,line_map, anddensity_map.Also modified default
zoomto be 0 instead of 8 (which we did not specify butpxsets this). A value of 0 will zoom out to the entire world, which makes more sense if you're not auto calculating thecenterbased on data.BREAKING CHANGE:
scatter_map,line_map, anddensity_mapnow have an explicit default zoom of 0 instead of an implicit default zoom of 8