-
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
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 |
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.
Removed
| ) | ||
|
|
||
| # 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 |
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.
Removed
| 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.
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.
I think that should be possible if done on the client (and using the JS API) and that was my future intention. I do think it's best to create another ticket for this so it can be properly designed as this is more of a quick fix. I just want to be intentional about it because, for example, we'll have to listen to user events on the map because if the user zooms, pans, etc. we don't want to keep auto centering.
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.
Created DH-21351
|
I went for very surface level, basic docs to start so I'll work on a data generator and adding more detail. |
|
plotly-express docs preview (Available for 14 days) |
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.
Pull request overview
This pull request fixes a bug with the center parameter in map-type charts and adds comprehensive documentation for map visualizations. The key changes include:
- Fixed the
centerparameter default handling to prevent auto-centering based on null data, which broke initial map views - Changed default
zoomfrom implicit 8 to explicit 0 (world view) for better initial visibility - Added two new data generator functions (
outages()andflights()) to support map examples - Created documentation for five map chart types with examples and API references
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
test_maps.py |
Updated test expectations to reflect new default center (0,0) and zoom (0) values |
maps.py |
Changed default zoom parameter from None to 0 and updated documentation strings |
_private_utils.py |
Added center parameter processing logic to set default (0,0) when None |
data_generators.py |
Added outages() and flights() data generator functions for map examples |
__init__.py |
Exported new data generator functions |
scatter-map.md |
Added documentation for scatter map visualizations |
scatter-geo.md |
Added documentation for scatter geo visualizations |
line-map.md |
Added documentation for line map visualizations |
line-geo.md |
Added documentation for line geo visualizations |
density-map.md |
Added documentation for density map visualizations (snapshot only) |
sidebar.json |
Added menu entries for new documentation pages |
*.json (snapshots) |
Added documentation snapshot files for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
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.
Change it to dx.data instead of dx_data. Styling questions for in demo this after.
Looks much better now overall though, thank you!
| lat="Lat", | ||
| lon="Lon", | ||
| zoom=9, | ||
| center={"lat": 44.97, "lon": -93.17} |
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.
These examples are much better, but it sucks that we have to provide the centre... has me yearning for DH-21259.
In Plotly's docs it looks like they have to provide the center as well: https://plotly.com/javascript/tile-density-heatmaps/
In their case looks like they just do somewhere in Africa so they can specify an easier to remember longitude (20)...
Can we at least provide this center as a constant in the data generator so we can just use that? e.g. dx.data.outages_center
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.
I think providing the center is something lots of people are going to want to do anyways so I want this example to be illustrative of how it needs to be set. What about dx.data.OUTAGE_LAT and dx.data.OUTAGE_LON?
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.
done
| from deephaven.plot.express import data as dx_data | ||
|
|
||
| # Load the outages dataset | ||
| outages_table = dx_data.outages() |
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.
This is inconsistent with the rest of the docs, just reference it using dx.data instead of assigning data to it's own variable.
| from deephaven.plot.express import data as dx_data | |
| # Load the outages dataset | |
| outages_table = dx_data.outages() | |
| # Load the outages dataset | |
| outages_table = dx.data.outages() |
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.
done
| outages_table, | ||
| lat="Lat", | ||
| lon="Lon", | ||
| color_discrete_sequence="black", |
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.
We should probably have different default colours for maps, as our colour is pretty close to that of water:
Make a ticket for that, unsure how easy it is to do a default for this and leave the other plots the same...
Or maybe we just want to default to map_style=dark, as that looks pretty cool with our default colours:
Let's ask Don in the demo this aft.
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.
Attempted to set based on theme in deephaven/web-client-ui#2605
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
|
||
| ### Use different projections and scopes | ||
|
|
||
| Change the map projection using the `projection` argument. Options include 'natural earth', 'mercator', and 'orthographic'. Adjust the geographic scope using the `scope` argument to focus on specific regions such as 'world', 'usa', 'europe', or 'north america'. Set the `center` argument for a better initial view, especially when scoping to a specific region. |
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.
would it be better to use double quotes since that is what is used in the example? avoids any possible confusion
|
plotly-express docs preview (Available for 14 days) |
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.
Styles look good, everything looks good but I'd also do a FLIGHT_CENTER and/or OUTAGES_CENTER, seems a little cleaner for the data set usage.
| lon="Lon", | ||
| zoom=9, | ||
| center={"lat": 44.97, "lon": -93.17} | ||
| center={"lat": dx.data.OUTAGE_LAT, "lon": dx.data.OUTAGE_LON} |
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.
I kinda just want OUTAGE_CENTER, would make it more concise...
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.
I suppose I don't oppose it that much. Easy enough to figure out the format. Done
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.
@jnumainville We should be more informative/restrictive with the center type anyway. We should use a TypedDict for the center type instead of just a dict[str, float].
| zoom=3, | ||
| center={"lat": 50, "lon": -100}, | ||
| map_style="carto-darkmatter" | ||
| center={"lat": dx.data.FLIGHT_LAT, "lon": dx.data.FLIGHT_LON}, |
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.
Ditto - FLIGHT_CENTER
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.
Done.
|
plotly-express docs preview (Available for 14 days) |
This doesn't really block deephaven/deephaven-plugins#1279 but is used for it. I do think it's best to allow anyone to override the `map_style` in the theme if we want it to be allowed in the theme, although this method is a bit wonky so I'm not sure if it's the best way to do it.
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. The default map style is now set based on the theme.