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

Add GeoJSON display class #10253

Merged
merged 1 commit into from Feb 7, 2017
Merged

Add GeoJSON display class #10253

merged 1 commit into from Feb 7, 2017

Conversation

@gnestor
Copy link
Contributor

gnestor commented Feb 7, 2017

@gnestor gnestor force-pushed the gnestor:geojson branch from 02a5368 to 37615ef Feb 7, 2017
@rgbkrk
rgbkrk approved these changes Feb 7, 2017
@rgbkrk
rgbkrk approved these changes Feb 7, 2017
@rgbkrk
Copy link
Member

rgbkrk commented Feb 7, 2017

Oops, I approved twice. It must be really good, since I also approved of it in the JupyterLab extension. 😉

@gnestor
Copy link
Contributor Author

gnestor commented Feb 7, 2017

I tested locally and it's working:

from IPython.display import GeoJSON

GeoJSON({
    "type": "Feature",
    "geometry": {
        "type": "Point",
        "coordinates": [-118.4563712, 34.0163116]
    },
    "properties": {
        "name": "Clover Park"
    }
})
@rgbkrk
Copy link
Member

rgbkrk commented Feb 7, 2017

Do we have tests for the HTML or other display functions that we could use here as well to make sure this gets covered?

@gnestor
Copy link
Contributor Author

gnestor commented Feb 7, 2017

Good call. There is a test_json that I could use for reference: https://github.com/ipython/ipython/blob/master/IPython/core/tests/test_display.py#L137-L164

@Carreau Carreau added this to the 6.0 milestone Feb 7, 2017
@Carreau Carreau merged commit 3ce26ed into ipython:master Feb 7, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ellisonbg
Copy link
Member

ellisonbg commented Feb 7, 2017

@Carreau
Copy link
Member

Carreau commented Feb 7, 2017

We might want to add that in the what's new. We can refine in subsequent PR.
ALso at some point we should try to register things with entry-points not to block on IPython releases.

@gnestor
Copy link
Contributor Author

gnestor commented Feb 7, 2017

ALso at some point we should try to register things with entry-points not to block on IPython releases.

@Carreau Can you clarify?

We might want to add that in the what's new.

I'm happy to do this. I assume a new release section will be added to version5.rst?

@rgbkrk
Copy link
Member

rgbkrk commented Feb 7, 2017

I'm so looking forward to the next IPython release now that we have updating displays and GeoJSON built-in. 🦅

@ellisonbg
Copy link
Member

ellisonbg commented Feb 7, 2017

@gnestor
Copy link
Contributor Author

gnestor commented Feb 7, 2017

@rgbkrk So I haven't been able to get display updates to work in notebook or lab. It's implemented in both, isn't it? I'm running them and ipython from master...

@rgbkrk
Copy link
Member

rgbkrk commented Feb 7, 2017

Should be in notebook, we certainly merged it. Has to be master of ipython, ipykernel, and notebook.

@ellisonbg
Copy link
Member

ellisonbg commented Feb 7, 2017

@Carreau
Copy link
Member

Carreau commented Feb 7, 2017

@Carreau Can you clarify?

Basically any update to the display mechanism need update to IPython itself that can take a year or so to get released. It would be better to provide a mechanism for other packages to provide this. In Python that would be entry-points.

Then you can "just" update/publish an ipython-geosjon, ipython-vegalite, ipython-mp3 , ipython-pdf , ipython-midi ... and have IPython auto discover these.

Then in case of updates needed, a new release of IPython is not necessary.

@ellisonbg
Copy link
Member

ellisonbg commented Feb 7, 2017

@Carreau
Copy link
Member

Carreau commented Feb 7, 2017

One other reason is that this is targeted as IPython 6.0, so only Python 3 users will have access to it.

@ellisonbg
Copy link
Member

ellisonbg commented Feb 7, 2017

@Carreau
Copy link
Member

Carreau commented Feb 7, 2017

Hmmm...good point. Overall, I don't mind if we have nice things in 6.0 that
aren't going to be available for users in python2. That is the carrot part
of moving people to python 3...

So maybe in this case I am ok with that.

I agree but, as someone have to play devil advocate this will put some extra weight on implementers of libraries if they want to be Py2 and Py3.

And to disagree with myself, anyway if py2/py3 is not a lot more complicated than checking IPython version, so I'm unsure it really matters.

@rgbkrk
Copy link
Member

rgbkrk commented Feb 7, 2017

A long time ago I remember voting in favor of Python3 only for the server, it's been annoying that we're not doing ipykernel and ipython fixes across 2 and 3. However, I haven't been putting maintainership cycles into ipython (except for some small things), so I feel I don't have much sway on this.

@gnestor
Copy link
Contributor Author

gnestor commented Feb 7, 2017

To come back around to the GeoJSON display class, currently the tile layer options are hard-coded in the renderer but perhaps they should be configurable from the GeoJSON class? E.g.

GeoJSON(data, tileUrlTemplate, tileLayerOptions)
@rgbkrk
Copy link
Member

rgbkrk commented Feb 7, 2017

To add on, we're thinking tileUrlTemplate: string and tileLayerOptions: dict because of the Leaflet options.

@gnestor
Copy link
Contributor Author

gnestor commented Feb 9, 2017

@ellisonbg Are you aware of a way to access the metadata property of an output from a mimerender extension? I updated the GeoJSON display class to accept additional options (url_template and layer_options) so it's in the metadata, I just don't know how to access on render... 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.