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

Geojson example #302

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Geojson example #302

wants to merge 11 commits into from

Conversation

deeplook
Copy link

Add example to render GeoJSON on a globe. The prose might still change a bit... All feedback is welcome!

@deeplook deeplook mentioned this pull request Mar 30, 2020
@coveralls
Copy link

coveralls commented Mar 30, 2020

Coverage Status

Coverage remained the same at 66.2% when pulling 13c01e8 on deeplook:geojson_example into baf03e7 on maartenbreddels:master.

@maartenbreddels
Copy link
Collaborator

Very cool! The gif is a bit large, you can try https://ezgif.com/resize to make it slightly smaller, they are also good at recompressing it.

I also wonder if we should have a sphere function in thee pylab module, what do you think? (not for this PR thought!). Another idea is to have a geo module, where some of the other functions could go (again, not this PR).

You say "It can render coordinates and lines, but not solid polygons." why is that?

I think we also need the geojson module in requirements_rtd.txt, and in fact, we should try to run the notebooks as part of the CI!

@deeplook
Copy link
Author

Very cool! The gif is a bit large, you can try https://ezgif.com/resize to make it slightly smaller, they are also good at recompressing it.

I'll try, but the blue sphere will likely show ugly artefacts. Something brighter might be better. Also, I've used points only and not lines on purpose, because there is no way to define the linewidth and they will appear too thin.

I also wonder if we should have a sphere function in thee pylab module, what do you think? (not for this PR thought!). Another idea is to have a geo module, where some of the other functions could go (again, not this PR).

Definitely! At first I used a "single sphere dot" (the wrapped ThreeJS primitive used for scatter series), but that seems not to be configurable in terms of number of facets and likely has no texture support. The initial code was actually contributed by @grburgess to https://github.com/deeplook/spheromones. It has some issue with with textures that I want to investigate. But, yes, absolutely.

You say "It can render coordinates and lines, but not solid polygons." why is that?

My research indicated that rendering polygons (mesh, triangulations...) on a sphere is not so trivial. If you know something that would be awesome.

I think we also need the geojson module in requirements_rtd.txt,

I can do without the geojson package if I use my own replacement for geojson.utils.coords which I already have. In fact it would be nicer to add something like geojson.utils.lines and (if we can render it) geojson.utils.polygons to geojson itself.

and in fact, we should try to run the notebooks as part of the CI!

Absolutely, I've done that in another project. But the question would be how well we an poke into the state of the created plots...

@maartenbreddels
Copy link
Collaborator

I'll try, but the blue sphere will likely show ugly artefacts.

maybe only decrease the resolution (since the gallery will make it smaller).

because there is no way to define the linewidth and they will appear too thin.

Annoying, we should have an implementation of threejs' Line, or maybe look at WebGL2.

I can do without the geojson package

I have no problem adding the dependency for RTD.

But the question would be how well we an poke into the state of the created plots...

We don't, we can upload the notebook un-executed, nbconvert will execute them for us at RTD.

@grburgess
Copy link

Ah cool, you are adding the earth sphere in here? I’m a bit behind today due to other projects and looking into the figure class

@deeplook
Copy link
Author

Ah cool, you are adding the earth sphere in here? I’m a bit behind today due to other projects and looking into the figure class

Yep, would be nice to add a texture, too, but I want to first investigate/fix the issue you know about... ;)

@grburgess
Copy link

@deeplook I'm interested to learn the fix. I did see some things about this when I was investigating the other day, but haven't had time to look. Great to see all the activity in ipv!

@deeplook
Copy link
Author

@maartenbreddels I've shrinked the size of geojson.gif which is now half that of wafe.gif.

@deeplook
Copy link
Author

deeplook commented Mar 31, 2020

@grburgess @maartenbreddels I've created this notebook to explore the "seam effect" on the back of taxtures mapped to sphere. It can be used to test certain hypothesis about patterns and their effects. I've played with it, but it doesn't give a clear hint. It could be an issue with the code of the sphere function itself, it could also be something deeply burried in ipyvolume's wrapping of threejs. Any further help is highly welcome.

https://gist.github.com/deeplook/d9ac96508362ce5eb5015a7e449eb0b6

Screenshot 2020-03-31 at 20 39 43

@maartenbreddels
Copy link
Collaborator

I tried to debug your function, but there is a wrapping going on at phi=pi, but I couldn't figure out why. What about this:

def sphere(x, y, z, radius, color="red", texture=None, num=400):
    """Create a sphere mesh with origin at x, y, z and radius.
    """
    assert num > 0
    nu = num
    nv = num
    u = np.linspace(0, 1, nu)
    v = np.linspace(0, 1, nv)
    u, v = np.meshgrid(u, v)
    phi = u * 2 * np.pi
    theta = v * np.pi
    x = x + radius * np.cos(phi) * np.sin(theta)
    y = y + radius * np.sin(phi) * np.sin(theta)
    z = z + radius * np.cos(theta)

    return ipv.plot_mesh(x, y, z, u=u, v=v, color=color, texture=texture, wireframe=False)

@deeplook
Copy link
Author

Look much better, indeed! Thanks!!

@deeplook
Copy link
Author

deeplook commented Mar 31, 2020

Except for a point reflection effect which I'm trying to fix myself. This effect disappears when I multiply phi and theta with -1 as below. (One can see that easier when using a world image map as texture.)

    phi = -u * 2 * np.pi
    theta = -v * np.pi

@deeplook
Copy link
Author

Still not quite there. Orientation of the sphere doesn't match that of loaded GeoJSON data (which changed right now, but it also didn't match before). Must take a brake from this for a short while...

@maartenbreddels
Copy link
Collaborator

point reflection effect

What do you mean by this, do you have a screenshot?

@deeplook
Copy link
Author

deeplook commented Apr 1, 2020

I've fiddled with it here: https://gist.github.com/deeplook/1a8cba10e3b27a27c8472417368dc425 (easier to download). But I guess I have to understand meshes better in order to match the xyz nad uv calculations with that used for extracting the GeoJSON coordinates.

Screenshot 2020-04-01 at 10 42 45

@grburgess
Copy link

Ah, yes, the UV mappings are different depending on the orientations. It took me a while to get it to Z...

@maartenbreddels
Copy link
Collaborator

I think texture coordinates with v=1 being the top, that would mean:

    return ipv.plot_mesh(x, y, z, u=u, v=1-v, **kwargs)

And, with trail and error :-D

ipv.scatter(-x, y, -z, size=0.5, color='limegreen', marker='sphere')

@deeplook
Copy link
Author

deeplook commented Apr 3, 2020

Got it... here with a sample use case... ;)

Screenshot 2020-04-03 at 11 44 52

@deeplook
Copy link
Author

deeplook commented Apr 3, 2020

@maartenbreddels The uv bug should be fixed now. So I'm also using a texture and will download it as well as the GeoJSON data from the net using ipyvolume's caching mechanism. If desired I could do more error checking at runtime (and license checking, too).

@maartenbreddels
Copy link
Collaborator

Please do, and could you rebase this branch?

@maartenbreddels maartenbreddels mentioned this pull request Apr 7, 2020
@deeplook
Copy link
Author

deeplook commented Apr 7, 2020

@maartenbreddels Good enough?

@maartenbreddels
Copy link
Collaborator

Looks good! (still not mergeable though)

But you should not merge master into this branch, but rebase instead, otherwise, history becomes complex. Happy to fix that for you btw!

@deeplook
Copy link
Author

deeplook commented Apr 7, 2020

I'm used to complex history. ;) But would be glad if you have the time to fix it.

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.

4 participants