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

ENH: support timezones and uuid in explore #3261

Merged

Conversation

m-richards
Copy link
Member

@m-richards m-richards commented Apr 23, 2024

Resolves #2378.

A bit uncertain about the api here. If we by default cast timestamps / objects to strings implicitly then users may not realise the performance hit associated with doing so (although I would expect if you are hitting a performance bottleneck in explore it is the time taken to render all the geometry on the javascript side, not any input prep we do).

We could expose a keyword controling behaviour of handling timestamp columns (coerce,skip,None), defaulting to None, which behaves as coerce, but perhaps warns about conversions happening when the input gdf has more than X rows?

We could also aim to get something simple working in for 1.0, and then consider if there are more performant options as a follow up.

There is also the option we could preserve additional geometry columns as WKT, rather than just drop them as we currently do.

@@ -323,6 +325,14 @@ def _colormap_helper(_cmap, n_resample=None, idx=None):
elif not gdf.crs.equals(4326):
gdf = gdf.to_crs(4326)

# Fields which are not JSON serializable are coerced to strings
Copy link
Member Author

Choose a reason for hiding this comment

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

I put this here near the linearring & crs conversions. Originally had it just before producing a featurecollection, but needs to happen before categorical detection happens - if the timestamp is column, then it needs to already be a string to be handled properly, otherwise bins = np.linspace(vmin, vmax, 257)[1:] crashes.

@martinfleis
Copy link
Member

I would do it exactly as you did in here. Automatically cast to strings, with no keywords. The explore API is already a bit large and if you really need to do something else, you can do it before calling explore. I would also not be worried about the performance hit, as explore is unusable for larger dataframes anyway.

There is also the option we could preserve additional geometry columns as WKT, rather than just drop them as we currently do.

That would make sense only for points as anything else would flood the tooltip with a long unreadable string. I'd hold on and do that only if someone feels it is needed.

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Not sure why codecov does not see those lines as covered as they clearly are. But I wouldn't worry about that.

Can you just add a changelog note?

@m-richards m-richards changed the title ENH:support timezones and uuid in explore ENH: support timezones and uuid in explore Apr 28, 2024
Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Thanks!

@jorisvandenbossche jorisvandenbossche merged commit 503e9be into geopandas:main May 18, 2024
19 of 20 checks passed
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.

ENH: explore(): skip if fields/index are Timestamp
3 participants