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

Improvement of annotators #583

Merged
merged 13 commits into from
Jan 16, 2023
Merged

Improvement of annotators #583

merged 13 commits into from
Jan 16, 2023

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Jul 19, 2022

Fixes #526
Fixes problems seen in Annotators.

Things done:

  • Added function transform_shapely_to_wsg84 as large coordinates shows a blank shapely figure. Also visible on the website.
Before After
image image
  • Converted string to pathlib.
    image

  • Update hover and link code with correct projection functions. Used this example.

  • Changed .opts to .options, though I'm not entirely sure if this is the correct fix.

Things not done:

TODO

  • Make unit test for transform_shapely_to_wsg84

@philippjfr
Copy link
Member

The geometry not being visible is definitely an issue but I'm not sure I love the fix. I would expect the geometry returned by the element to match the crs of that element. Is this a known issue in shapely or do large coordinate values actually get stored incorrectly causing the issue?

@hoxbro
Copy link
Member Author

hoxbro commented Jul 26, 2022

The SVG created by shapely doesn't show up in the notebook. It seems to be related to Jupyter as far as I can see:

If I open the saved file in Inkscape, it renders correctly:

It seems to be a known issue: shapely/shapely#574

Code
from IPython.display import SVG, display
from shapely.geometry import MultiPoint, Point

yy = [-10131185, -10131943, -10131766, -10131032]
xx = [3805587, 3803182, 3801073, 3799778]

points = [Point(x, y) for (x, y) in zip(xx, yy)]
multi = MultiPoint(points)

multi._repr_svg_()

with open("tmp.svg", "w") as f:
    f.write(multi._repr_svg_())

@philippjfr
Copy link
Member

Thanks for investigating that. If it's a known bug in shapely I'm a strong -1 in changing our behavior. What I would be in favor of is to add a projection argument to the geom methods that lets you easily project the geometry to some other coordinate system, e.g. WGS84. Then we can update the notebooks so they don't look broken while maintaining the existing correct default behavior.

@hoxbro
Copy link
Member Author

hoxbro commented Jul 26, 2022

Completely agree with you.

I have implemented this suggestion. I have removed the flip from transform_shapely, which means that the output of .geom() is rotated compared to the bokeh plot. I can add this back.

image

Another thing is I added an extra point in the first example to avoid #584.

@philippjfr
Copy link
Member

Hmm, I guess I'm confused about that. Why is the svg rotated? When I load shapely objects via geopandas for example they use the same convention for storing x/y that we do and do not appear to be flipped.

@hoxbro
Copy link
Member Author

hoxbro commented Jul 26, 2022

After some digging this is because I use wgs84 instead of PlateCarree(), which I thought was the same thing. Should an option of setting projection=True use PlateCarree or will this be too complicated?

image

@hoxbro
Copy link
Member Author

hoxbro commented Jan 6, 2023

Issue #584 is also a problem with 5 points. I will update the issue with more information.

@hoxbro hoxbro requested a review from philippjfr January 6, 2023 11:09
@hoxbro hoxbro added this to the Version 1.9.6 milestone Jan 14, 2023
Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is a big improvement.

@philippjfr philippjfr merged commit c03cbc7 into main Jan 16, 2023
@philippjfr philippjfr deleted the fix_annotators branch January 16, 2023 17:17
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.

annotators wont work in geoviews
2 participants