scverse / spatialdata

An open and interoperable data framework for spatial omics data
https://spatialdata.scverse.org/
BSD 3-Clause "New" or "Revised" License
236 stars 45 forks source link

`transform()` function rotates shapes incorrectly -> incorrect extend #353

Closed timtreis closed 1 year ago

timtreis commented 1 year ago

MRE

import geopandas as gpd
import spatialdata
import spatialdata_plot
from shapely import MultiPolygon, Point, Polygon

points = []
for p in [[0, 0], [0, 1], [1, 1], [1, 0]]:
    points.append(Point(p))

gdf = gpd.GeoDataFrame(geometry=points)
gdf["radius"] = 0.05
circles_df = ShapesModel.parse(gdf)
sdata = spatialdata.SpatialData(shapes={"circles": circles_df})

theta = math.pi / 4
rotation = Affine(
[
    [math.cos(theta), -math.sin(theta), 0],
    [math.sin(theta), math.cos(theta), 0],
    [0, 0, 1],
],
input_axes=("x", "y"),
output_axes=("x", "y"),
)
set_transformation(element=sdata["circles"], transformation=rotation, to_coordinate_system="transformed")

sdata.pl.render_shapes().pl.show()

for cs in ["global", "transformed"]:
    print(cs, get_extent(sdata, coordinate_system=cs))

Produces the following plot:

image

We expect the axis of the transformed plot to also touch the circles. This is not the case since the extent is calculated to transformed {'y': (-0.07071067811865475, 1.48492424049175), 'x': (-0.7778174593052023, 0.7778174593052024)}.

However, the error doesn't come from the extent calculation, but from L305 in _compute_extent_in_coordinate_system:

image

Bounding box before and after transformation:

image

Since we're rotating by 45°, we'd expect them to be

[ 0,      0]
[−0.7071, 0.7071]
[ 0,      1.4142]
[ 0.7071, 0.7071]

(+/- the radius of the circle 0.05) which would result in a different bounding box. With larger radii, the inaccuracy becomes bigger

radius = 0.5

image
LucaMarconato commented 1 year ago

Quick question @timtreis. Running the code produces this: image

I think I need to use one of the new branches of spatialdata-plot that will make it into the next release. Which one?

LucaMarconato commented 1 year ago

I guess https://github.com/scverse/spatialdata-plot/pull/162, gonna try now.

timtreis commented 1 year ago

Yes, this one. Can you reproduce?

LucaMarconato commented 1 year ago

Yes thanks. This is the cause of the bug:

image
timtreis commented 1 year ago

Ahhh yeah, makes sense. That's why it scales with r

LucaMarconato commented 1 year ago

The current approach works for raster data, the affected part is points, shapes and polygons. I will implement the exact calculation for the vector data, enabled by default, but I will also make it optional with a parameter of get_extent.

I suspect that the current implementation can be a lot faster since it doesn't need to transform the data, and for most cases actually one doesn't need the exact extent.

LucaMarconato commented 1 year ago

Fixed image Gonna make a PR to the get_extent() PR, I will also fix the failing tests.

LucaMarconato commented 1 year ago

I am adding tests, not just for circles but also for points, polygons and multipolygons.

These are the plots that one would get with the old code (note that I have changed the positions of the circles in the "global" coordinate system). 4 circles image 4 polygons image 4 multipolygons image 4 points image

This is with the new code. 4 circles image 4 polygons image 4 multipolygons image 4 points image

Things work, except for points. I will:

LucaMarconato commented 1 year ago

All correctly computed. I have opened the aforementioned issues in spatialdata-plot and I have made a PR to merge against the get_extent() PR.

LucaMarconato commented 1 year ago

Fixed by https://github.com/scverse/spatialdata/pull/373 and merged via https://github.com/scverse/spatialdata/pull/318