-
-
Notifications
You must be signed in to change notification settings - Fork 271
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
Add GeoJSON support #876
Add GeoJSON support #876
Conversation
d1ecebb
to
13d3b98
Compare
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.
Hi @dracos, thanks for putting this together! Overall it looks good, just a few changes and questions.
dev/formats.jsonl
Outdated
@@ -18,7 +18,7 @@ | |||
{"filetype": "ttf", "filetype_url": "#ttf", "aliases": "otf", "requirements": "fonttools", "format": "TrueType Font", "VisiData loader": "yes", "VisiData saver": "", "version_added": "1.1", "created": "1991", "creator": "Apple", "description": "", "open format": "yes", "nestable": "", "plottable": "plottable", "format_url": "https://en.wikipedia.org/wiki/TrueType"} | |||
{"filetype": "dot", "filetype_url": "#pcap", "aliases": "", "requirements": "", "format": "Graphviz diagram", "VisiData loader": " ", "VisiData saver": "from pcap", "version_added": "1.2", "created": "1991", "creator": "", "description": "output for pcap only", "open format": "yes", "nestable": "", "plottable": "", "format_url": "https://graphviz.gitlab.io/_pages/doc/info/lang.html"} | |||
{"filetype": "dta", "aliases": "", "requirements": "pandas", "format": "Stata", "VisiData loader": "yes", "VisiData saver": "", "version_added": "1.2", "created": "1985", "creator": "StataCorp", "description": "", "open format": "no", "nestable": "", "plottable": "", "format_url": "https://www.loc.gov/preservation/digital/formats/fdd/fdd000471.shtml"} | |||
{"filetype": "geojson", "filetype_url": "#shp", "aliases": "", "requirements": "", "format": "Geographic JSON", "VisiData loader": " ", "VisiData saver": "from shp", "version_added": "1.2", "created": "2008", "creator": "", "description": "derivative of JSON", "open format": "yes", "nestable": "", "plottable": "plottable", "format_url": "http://geojson.org/"} | |||
{"filetype": "geojson", "filetype_url": "#shp", "aliases": "", "requirements": "", "format": "Geographic JSON", "VisiData loader": "yes", "VisiData saver": "", "version_added": "1.2", "created": "2008", "creator": "", "description": "derivative of JSON", "open format": "yes", "nestable": "", "plottable": "plottable", "format_url": "http://geojson.org/"} |
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.
Thanks for remembering this! Let's also change the version_added to 2.2 (which is the next release).
visidata/loaders/geojson.py
Outdated
self.colnames[prop] = c | ||
self.addColumn(c) | ||
|
||
yield from Progress(features) |
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.
Let's yield feature
in the first pass instead of doing two passes over features
(and move the Progress into that for
loop).
for feature in features: | ||
for prop in feature.get('properties', {}).keys(): | ||
if prop not in self.colnames: | ||
c = Column(name=prop, getter=getter_factory(prop)) |
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.
You should be able to use ItemColumn
with a nested name: "properties."+prop
.
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.
AttrColumn handles nested properties, but these are normal dicts, and ItemColumn does not handle that.
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.
You are right! It really seems like it should, sorry to give you the runaround. This again tempts me to make it nestable, even at the cost of having it not work with dict keys that have .
in them. But that's not for tonight, so we can keep the getter_factory for now.
visidata/loaders/geojson.py
Outdated
self.reset() | ||
|
||
for row in Progress(self.sourceRows): | ||
k = tuple(col.getValue(row) for col in self.source.keyCols) |
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.
Use self.source.rowkey(row)
.
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'll update the shp
loader as well, presumably?
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.
Yes, please! Thanks for helping to keep it clean.
visidata/loaders/geojson.py
Outdated
if (isinstance(vs, ShapeMap)): | ||
return save_shp_geojson(vd, p, vs) | ||
|
||
isinstance(vs, Canvas) or vd.fail("must save GeoJSON from map sheet") |
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 only works for GeoJSONMap (not all Canvas). With the change in decorators we can remove this.
visidata/loaders/shp.py
Outdated
@@ -71,8 +73,7 @@ def reload(self): | |||
|
|||
self.refresh() | |||
|
|||
@VisiData.api | |||
def save_geojson(vd, p, vs): | |||
def save_shp_geojson(vd, p, vs): |
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 should actually be save_geojson
with @ShapeMap.api
decorator.
visidata/loaders/shp.py
Outdated
@@ -83,7 +84,7 @@ def save_geojson(vd, p, vs): | |||
'coordinates': [[x, y] for x, y in coords], | |||
}, | |||
'properties': { | |||
col.name: col.getTypedValue(row) for col in vs.source.visibleCols | |||
col.name: col.getTypedValue(row) for col in vs.source.keyCols |
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 the one change I'm not sure of...don't we want all the visibleCols as saved properties? Does some other GeoJSON tool balk at unneeded columns?
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.
Not sure what I thought here, I guess I confused key with visible. Will revert
visidata/loaders/geojson.py
Outdated
|
||
@VisiData.api | ||
def save_geojson(vd, p, vs): | ||
if (isinstance(vs, ShapeMap)): |
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 isn't needed if we use the @GeoJSONMap.api
decorator (instead of @VisiData.api`). Then we'd also change the original save_geojson (noted below).
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 made this change (and similar shp
one), but I now get "no function to save as type geojson" on both maps.
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.
Hm, that should have worked. Unfortunately I'm out of steam for tonight. I'll take a look at this tomorrow and we'll hopefully get this merged just in time for 2.2!
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.
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.
Thanks! Yes, that seems to have worked.
visidata/loaders/geojson.py
Outdated
fp.write(chunk) | ||
|
||
GeoJSONSheet.addCommand('.', 'plot-row', 'vd.push(GeoJSONMap(name+"_map", sheet, sourceRows=[cursorRow], textCol=cursorCol, source=sheet))', 'plot geospatial vector in current row') | ||
GeoJSONSheet.addCommand('g.', 'plot-rows', 'vd.push(GeoJSONMap(name+"_map", sheet, sourceRows=rows, textCol=cursorCol, source=sheet))', 'plot all geospatial vectors in current sheet') |
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.
What do you think about changing g.
to be plot-selected
and have them use someSelectedRows
?
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, g.
to me implies global-plot. I'd say if anything .
on its own should plot current row if no selection, or selected rows if some selected.
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 see what you mean. To be honest, plotting only the current row has never been very useful, so I wonder if .
itself should be someSelectedRows, and g.
should always be "all rows". That wouldn't be consistent with most of the TableSheet g
bindings though. Thoughts for another PR 🧷
visidata/loaders/geojson.py
Outdated
|
||
GeoJSONSheet.addCommand('.', 'plot-row', 'vd.push(GeoJSONMap(name+"_map", sheet, sourceRows=[cursorRow], textCol=cursorCol, source=sheet))', 'plot geospatial vector in current row') | ||
GeoJSONSheet.addCommand('g.', 'plot-rows', 'vd.push(GeoJSONMap(name+"_map", sheet, sourceRows=rows, textCol=cursorCol, source=sheet))', 'plot all geospatial vectors in current sheet') | ||
InvertedCanvas.addCommand('^S', 'save-sheet', 'vd.saveSheets(inputPath("save to: ", value=getDefaultSaveName(sheet)), sheet, confirm_overwrite=options.confirm_overwrite)', 'save current sheet to filename in format determined by extension (default .tsv)') |
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.
Maybe ultimately this should go on BaseSheet, but for now I think this should be on GeoJSONSheet specifically and not all InvertedCanvas.
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.
GeoJSONSheet already has a save-sheet on it, do you mean GeoJSONMap?
This adds loading and saving support for GeoJSON files, and fixes the Shapefile GeoJSON saving.
3683dbe
to
7cb0ac6
Compare
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.
Thanks again @dracos!
@dracos thank you very much! How view a geojson in vd? |
@aborruso Update your installed VisiData to the most recent develop! There will be a release sometime this week. =) |
How did you open it, and what is its filename? Its filename needs to end with |
Hi @dracos thank you again. I have opened it using And it's the same if I use I have |
@aborruso I think you are likely referencing an earlier commit of VisiData 2.2dev in your environment. =( |
@anjakefala I'm installing it using |
@aborruso I would try to completely uninstall all versions of VisiData ( |
Using |
As per the instructions I gave in the PR body, ( |
Yay @aborruso 💜!! I am so glad. |
This PR hopefully adds GeoJSON support, in much the same way as Shapefiles.
I had to add another save-sheet command to the canvas so that you could save from it, that didn't appear to work on Shapefiles previously.
https://github.com/martinjc/UK-GeoJSON/blob/master/json/administrative/wal/lad.json opened with
vd -f geojson lad.json
and selecting the English name column as a key column gives:And pressing
g.
with the English name column highlighted gives:Hope that's useful.