-
Notifications
You must be signed in to change notification settings - Fork 794
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
accept __geo_interface__ attribute as data #1664
accept __geo_interface__ attribute as data #1664
Conversation
Looks good - do you have an example of how using this would look in practice? |
Here are some examples of possible tests. Not all tests are passing yet. Also some advice how and where to implement these tests within Altair would be appreciated. import altair as alt
def geom_obj(geom):
class Geom(object):
pass
geom_obj = Geom()
setattr(geom_obj, '__geo_interface__', geom)
return geom_obj geom_a = {
"coordinates": [[
(0, 0),
(0, 2),
(2, 2),
(2, 0),
(0, 0)
]],
"type": "Polygon"
}
feat_a = geom_obj(geom_a)
# correct translation of Polygon geometry to Feature type
alt.Chart(feat_a).mark_geoshape(tooltip={"content": "data"}) geom_b = {
"geometry": {
"coordinates": [[
[6.90, 53.48],
[5.98, 51.85],
[6.07, 53.51],
[6.90, 53.48]
]],
"type": "Polygon"
},
"id": None,
"properties": {},
"type": "Feature"
}
feat_b = geom_obj(geom_b)
# removal of empty `properties` key
alt.Chart(feat_b).mark_geoshape(tooltip={"content": "data"}) geom_c = {
"geometry": {
"coordinates": [[
[6.90, 53.48],
[5.98, 51.85],
[6.07, 53.51],
[6.90, 53.48]
]],
"type": "Polygon"
},
"id": None,
"properties": {"country": "Spain"},
"type": "Feature"
}
feat_c = geom_obj(geom_c)
# correct registration of `country` as foreign member
alt.Chart(feat_c).mark_geoshape(tooltip={"content": "data"}) import array as arr
geom_d = {
"bbox": arr.array('d', [1.1, 3.5, 4.5]),
"geometry": {
"coordinates": [tuple((
tuple((6.90, 53.48)),
tuple((5.98, 51.85)),
tuple((6.07, 53.51)),
tuple((6.90, 53.48))
))],
"type": "Polygon"
},
"id": 27,
"properties": {},
"type": "Feature"
}
feat_d = geom_obj(geom_d)
# serializing of arrays to lists
# serializing of (nested) tuples to (nested) lists
# removal of empty `properties` key
alt.Chart(feat_d).mark_geoshape(tooltip={"content": "data"}) geom_d = {
"geometry": {
"coordinates": [[
[6.90, 53.48],
[5.98, 51.85],
[6.07, 53.51],
[6.90, 53.48]
]],
"type": "Polygon"
},
"id": 27,
"properties": {"type": "foo"},
"type": "Feature"
}
feat_d = geom_obj(geom_d)
# cannot draw geoshape
# incorrect registration of `type` as foreign member
alt.Chart(feat_d).mark_geoshape(tooltip={"content": "data"}) # geopandas can handle unicode characters in __geo_interface__
import geopandas as gpd
fp_earth = gpd.datasets.get_path('naturalearth_lowres')
gdf = gpd.read_file(fp_earth)
gpd_geo_interface = gdf.__geo_interface__ # shapefile cannot handle unicode characters in __geo_interface__
# not the problem of altair
# pip install pyshp
import shapefile
sf = shapefile.Reader(fp_earth)
sf_geo_interface = sf.__geo_interface__
# GeoJSON’s RFC 7946 winding order is opposite compare to d3-geo (vega, vega-lite, altair)
# use geopandas if this is problematic # pip install Shapely==1.7a2
from shapely.ops import orient
gdf_sa = gdf[gdf.name=='South Africa']
gdf_sa_ccw = gdf_sa.geometry.apply(orient, args=(-1,))
# correct winding order
# exterior shell is counterclockwise and interionr rings are clockwise
alt.Chart(gdf_sa_ccw).mark_geoshape().project(type='mercator') gdf_sa_cw = gdf_sa.geometry.apply(orient, args=(1,))
# incorrect winding order
# exterior shell is clockwise and interior rings are counterclockwise
alt.Chart(gdf_sa_cw).mark_geoshape(tooltip={"content": "data"}).project(type='mercator', reflectY=True) # test using json data transformer
alt.data_transformers.enable('json') gdf_na = gdf[gdf.continent=='North America']
# serialize Feature Collections
# using data transformer json
alt.Chart(gdf_na).mark_geoshape().project(type='mercator') alt.data_transformers.enable('data_server') # using data transformer data_server
alt.Chart(gdf_na).mark_geoshape().project(type='mercator') |
None of our tests do any visual output comparison: they're all based on the generated chart specifications. So the right way to test these, I think, is to create a series of objects with |
@jakevdp I've added several tests plus some paragraphs of documentation. If some parts are a bit confusing, there is this issue: vega/vega#1319 on the Vega repo that might explain a bit more on the adopted approach for serialization. Basically its an attempt to implement this comment in the linked issue:
And generally speaking this works, as long as you don't use the column names Please feel free to share your thoughts. |
The more I think about it, the more I’m not liking it. Instead of un-nesting the entries of |
OK - happy to review that version if you think it would be better 😁 |
@jakevdp this is ready for review. |
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.
Looks great! Thanks for the work on this. A few comments inline
altair/utils/core.py
Outdated
""" | ||
|
||
try: | ||
feat['properties'].update({k: feat[k] for k in ('type', 'geometry')}) |
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.
It looks like {k: feat[k] for k in ('type', 'geometry')}
could be done above the try/except, since it happens in both blocks.
|
||
|
||
@contextmanager | ||
def not_raises(ExpectedException): |
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 would remove this context manager and all its uses, and let the unit test framework handle any exceptions that are raised.
doc/user_guide/data.rst
Outdated
|
||
- as a `Pandas DataFrame <http://pandas.pydata.org/pandas-docs/stable/generated/pandas.DataFrame.html>`_ | ||
- as a :class:`Data` or related object (i.e. :class:`UrlData`, :class:`InlineData`, :class:`NamedData`) | ||
- as a url string pointing to a ``json`` or ``csv`` formatted text file | ||
- as an object that supports the `__geo_interface__` |
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.
List some examples of data types that define ``geo_interface`
Updated as requested |
Looks good. One final comment if you want to make the change; you can avoid having to look up with alt.data_transformers.enable(consolidate_datasets=False):
spec = chart.to_dict()
data = spec['data'] |
👍 done! |
Awesome! Thanks for all your work on this! |
Let me enjoy this enlightening experience by closing #588 |
oh boy oh boy oh boy oh boy thank you! |
Could you clarify exactly what parts of the geo interface spec are used and how the data must be structured? I assume you must pass full Features, not just Geometrys, so that properties/attributes for classification and coloring are included? Does the data object passed to |
Any object that supports the The {"type": "Feature", "geometry": []} or a list of Features [
{"type": "Feature", "geometry": []},
{"type": "Feature", "geometry": []}
] Since a single geometry is valid under the geo_interface it is sanitized to a Feature without properties (covered by this test). A list of objects where only the objects in the list contains the Fiona does not (yet) support the
This might change in the future since you opened an issue on the fiona repo. ¹ in topojson I do accept lists where only the objects support the |
I'm not sure if I've understood you correctly here: #588 (comment)
But in this PR I tried to integrate serialising data objects with a
__geo_interface__
attribute.And it worked OK for Python packages that deal with geographical data types that support the
__geo_interface__
. I tried the packagesshapely
,pyshp
,geojson
,geopandas
.It works for:
InlineData
json
data transformer anddata_server
data transformer.See animated gif:
After I got this working, I compared it to https://github.com/altair-viz/altair/pull/818/files, but the idea is very much the same. Maybe @iliatimofeev can shed a light on this proof of concept as well.