unify polygons and shapes elements

[giovp]

New Shapes model

This PR unifies polygons and shape elements in a single region element: Shapes.

Motivation

As of now, the Shapes element was stored as an AnnData, with a bunch of metadata to keep track of the type of shapes and size, and with the centroid coordinates store in in adata.obsm["spatial"]. Thinking about it more, it was very much unnecessary:

• since it's a region element, only minimal amount of information should be stored there, as any annotations would go into Tables -> because of this, the AnnData format is unneccesary
• it contained a lot of unnecessary metadata for determining which type of shapes are stored (circle, square only so far).
• it could be simplified by representing it as a geopandas dataframe.

Solution

Since a geopandas.GeoDataFrame is probably the better way to represent Circles and Squares shapes, instead of AnnData, it naturally follows that polygons and shapes should be grouped together. Benefits are the following:

• unified Shapes element (less complexity)
• less metadata, offload to shapely handling of different geometry types
• easier access to aggregation functions for Circles from geopandas.

e.g. example for counting points under circles regions @ivirshup

importgeopandasasgpdfromspatialdata._core.modelsimportShapesModelfromspatialdata._core.modelsimportPointsModelcircles=ShapesModel.parse(np.array([[10, 2], [5, 2]]), geometry=0, radius=[2, 3])
points=PointsModel.parse(
np.array([[11, 3], [5, 3]]), annotation=pd.DataFrame({"a": [0, 1]})
)
spot=circles["geometry"].buffer(distance=circles["radius"])[0]
gpd.points_from_xy(**points[["x", "y"]].compute()).within(spot)

Summary

Changes are minimal:

• The Polygons element (and models, IO, etc.) it's now called Shapes and it also contains Circles
• Circles are represented as a geopandas.GeoDataFrame of shapely.Point with additional column for radius
• The old Shapes element is removed
• No additional metadata is introduced, only change from IO side is read/write for radius column in the case of Circles

[giovp]

tests in transforms are failing but I'd wait for #100 to be merged first before fixing, as there will be many changes to be made as well.

[LucaMarconato]

@giovp all tests are passing now. I did just fixes, no real change. Anyway if you want to review I suggest to diff
70d3f16 (only merged the branches and ran the pre-commits)
with the latest commit.

[codecov]

Codecov Report

Merging #132 (3017790) into main (56179d6) will decrease coverage by 0.33%.
The diff coverage is 81.44%.

Additional details and impacted files

@@ Coverage Diff @@## main #132 +/- ##
==========================================
- Coverage 87.51% 87.19% -0.33% 
==========================================
Files 22 22 Lines 3108 3014 -94 ==========================================
- Hits 2720 2628 -92 + Misses 388 386 -2 

Impacted Files	Coverage Δ
spatialdata/__init__.py	100.00% <ø> (ø)
spatialdata/_io/__init__.py	100.00% <ø> (ø)
spatialdata/_core/_spatialdata.py	75.68% <60.00%> (-1.17%)	⬇️
spatialdata/_core/_spatial_query.py	77.21% <62.50%> (ø)
spatialdata/_core/models.py	85.83% <80.00%> (+0.04%)	⬆️
spatialdata/_io/format.py	88.23% <83.33%> (+0.60%)	⬆️
spatialdata/_core/_transform_elements.py	85.81% <100.00%> (-1.03%)	⬇️
spatialdata/_core/core_utils.py	92.39% <100.00%> (-2.93%)	⬇️
spatialdata/_io/read.py	97.46% <100.00%> (-0.20%)	⬇️
spatialdata/_io/write.py	96.05% <100.00%> (-0.23%)	⬇️
... and 1 more

[kevinyamauchi]

This LGTM! I think it makes sense to unify the Circles and polygons here. I left a couple of minor comments below.

[kevinyamauchi]

what are the 0, 3, and 6 here?

[giovp]

thse are defined by GeometryType enum by shapely and are used to set the geometry type in the parser.

[giovp]

@LucaMarconato thanks a lot for fixing tests!!! I think we can go on and merge this, one comment:

• I saw that pyarrow was re-added, do we need it? Think it's an artefact right?

[LucaMarconato]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

[giovp]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

wait, if it's for the cosmx reader then it should be in spatialdata io. There is no other way for the repr to not use pyarrow?

EDIT: wasn't needed to I've removed it again

EDIT EDIT: I was wrong, but do we really want to keep the pyarrow.read_table just for the repr?

[LucaMarconato]

@giovp yes for the moment better to keep it. Dask will allow to get the length of the dataframe soon (dask/dask#9973), at that point we can remove the dependency.

[ivirshup]

@LucaMarconato I don't think that dask is going to get this soon. I don't see an active PR for it at the moment, and it's been open for a few years now.

[giovp]

New Shapes model

This PR unifies polygons and shape elements in a single region element: Shapes.

Motivation

As of now, the Shapes element was stored as an AnnData, with a bunch of metadata to keep track of the type of shapes and size, and with the centroid coordinates store in in adata.obsm["spatial"]. Thinking about it more, it was very much unnecessary:

• since it's a region element, only minimal amount of information should be stored there, as any annotations would go into Tables -> because of this, the AnnData format is unneccesary
• it contained a lot of unnecessary metadata for determining which type of shapes are stored (circle, square only so far).
• it could be simplified by representing it as a geopandas dataframe.

Solution

Since a geopandas.GeoDataFrame is probably the better way to represent Circles and Squares shapes, instead of AnnData, it naturally follows that polygons and shapes should be grouped together. Benefits are the following:

• unified Shapes element (less complexity)
• less metadata, offload to shapely handling of different geometry types
• easier access to aggregation functions for Circles from geopandas.

e.g. example for counting points under circles regions @ivirshup

importgeopandasasgpdfromspatialdata._core.modelsimportShapesModelfromspatialdata._core.modelsimportPointsModelcircles=ShapesModel.parse(np.array([[10, 2], [5, 2]]), geometry=0, radius=[2, 3])
points=PointsModel.parse(
np.array([[11, 3], [5, 3]]), annotation=pd.DataFrame({"a": [0, 1]})
)
spot=circles["geometry"].buffer(distance=circles["radius"])[0]
gpd.points_from_xy(**points[["x", "y"]].compute()).within(spot)

Summary

Changes are minimal:

• The Polygons element (and models, IO, etc.) it's now called Shapes and it also contains Circles
• Circles are represented as a geopandas.GeoDataFrame of shapely.Point with additional column for radius
• The old Shapes element is removed
• No additional metadata is introduced, only change from IO side is read/write for radius column in the case of Circles

[giovp]

tests in transforms are failing but I'd wait for #100 to be merged first before fixing, as there will be many changes to be made as well.

[LucaMarconato]

@giovp all tests are passing now. I did just fixes, no real change. Anyway if you want to review I suggest to diff
70d3f16 (only merged the branches and ran the pre-commits)
with the latest commit.

[codecov]

Codecov Report

Merging #132 (3017790) into main (56179d6) will decrease coverage by 0.33%.
The diff coverage is 81.44%.

Additional details and impacted files

@@ Coverage Diff @@## main #132 +/- ##
==========================================
- Coverage 87.51% 87.19% -0.33% 
==========================================
Files 22 22 Lines 3108 3014 -94 ==========================================
- Hits 2720 2628 -92 + Misses 388 386 -2 

Impacted Files	Coverage Δ
spatialdata/__init__.py	100.00% <ø> (ø)
spatialdata/_io/__init__.py	100.00% <ø> (ø)
spatialdata/_core/_spatialdata.py	75.68% <60.00%> (-1.17%)	⬇️
spatialdata/_core/_spatial_query.py	77.21% <62.50%> (ø)
spatialdata/_core/models.py	85.83% <80.00%> (+0.04%)	⬆️
spatialdata/_io/format.py	88.23% <83.33%> (+0.60%)	⬆️
spatialdata/_core/_transform_elements.py	85.81% <100.00%> (-1.03%)	⬇️
spatialdata/_core/core_utils.py	92.39% <100.00%> (-2.93%)	⬇️
spatialdata/_io/read.py	97.46% <100.00%> (-0.20%)	⬇️
spatialdata/_io/write.py	96.05% <100.00%> (-0.23%)	⬇️
... and 1 more

[kevinyamauchi]

This LGTM! I think it makes sense to unify the Circles and polygons here. I left a couple of minor comments below.

[kevinyamauchi]

what are the 0, 3, and 6 here?

[giovp]

thse are defined by GeometryType enum by shapely and are used to set the geometry type in the parser.

[giovp]

@LucaMarconato thanks a lot for fixing tests!!! I think we can go on and merge this, one comment:

• I saw that pyarrow was re-added, do we need it? Think it's an artefact right?

[LucaMarconato]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

[giovp]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

wait, if it's for the cosmx reader then it should be in spatialdata io. There is no other way for the repr to not use pyarrow?

EDIT: wasn't needed to I've removed it again

EDIT EDIT: I was wrong, but do we really want to keep the pyarrow.read_table just for the repr?

[LucaMarconato]

@giovp yes for the moment better to keep it. Dask will allow to get the length of the dataframe soon (dask/dask#9973), at that point we can remove the dependency.

[ivirshup]

@LucaMarconato I don't think that dask is going to get this soon. I don't see an active PR for it at the moment, and it's been open for a few years now.

[giovp]

New Shapes model

This PR unifies polygons and shape elements in a single region element: Shapes.

Motivation

As of now, the Shapes element was stored as an AnnData, with a bunch of metadata to keep track of the type of shapes and size, and with the centroid coordinates store in in adata.obsm["spatial"]. Thinking about it more, it was very much unnecessary:

• since it's a region element, only minimal amount of information should be stored there, as any annotations would go into Tables -> because of this, the AnnData format is unneccesary
• it contained a lot of unnecessary metadata for determining which type of shapes are stored (circle, square only so far).
• it could be simplified by representing it as a geopandas dataframe.

Solution

Since a geopandas.GeoDataFrame is probably the better way to represent Circles and Squares shapes, instead of AnnData, it naturally follows that polygons and shapes should be grouped together. Benefits are the following:

• unified Shapes element (less complexity)
• less metadata, offload to shapely handling of different geometry types
• easier access to aggregation functions for Circles from geopandas.

e.g. example for counting points under circles regions @ivirshup

importgeopandasasgpdfromspatialdata._core.modelsimportShapesModelfromspatialdata._core.modelsimportPointsModelcircles=ShapesModel.parse(np.array([[10, 2], [5, 2]]), geometry=0, radius=[2, 3])
points=PointsModel.parse(
np.array([[11, 3], [5, 3]]), annotation=pd.DataFrame({"a": [0, 1]})
)
spot=circles["geometry"].buffer(distance=circles["radius"])[0]
gpd.points_from_xy(**points[["x", "y"]].compute()).within(spot)

Summary

Changes are minimal:

• The Polygons element (and models, IO, etc.) it's now called Shapes and it also contains Circles
• Circles are represented as a geopandas.GeoDataFrame of shapely.Point with additional column for radius
• The old Shapes element is removed
• No additional metadata is introduced, only change from IO side is read/write for radius column in the case of Circles

[giovp]

tests in transforms are failing but I'd wait for #100 to be merged first before fixing, as there will be many changes to be made as well.

[LucaMarconato]

@giovp all tests are passing now. I did just fixes, no real change. Anyway if you want to review I suggest to diff
70d3f16 (only merged the branches and ran the pre-commits)
with the latest commit.

[codecov]

Codecov Report

Merging #132 (3017790) into main (56179d6) will decrease coverage by 0.33%.
The diff coverage is 81.44%.

Additional details and impacted files

@@ Coverage Diff @@## main #132 +/- ##
==========================================
- Coverage 87.51% 87.19% -0.33% 
==========================================
Files 22 22 Lines 3108 3014 -94 ==========================================
- Hits 2720 2628 -92 + Misses 388 386 -2 

Impacted Files	Coverage Δ
spatialdata/__init__.py	100.00% <ø> (ø)
spatialdata/_io/__init__.py	100.00% <ø> (ø)
spatialdata/_core/_spatialdata.py	75.68% <60.00%> (-1.17%)	⬇️
spatialdata/_core/_spatial_query.py	77.21% <62.50%> (ø)
spatialdata/_core/models.py	85.83% <80.00%> (+0.04%)	⬆️
spatialdata/_io/format.py	88.23% <83.33%> (+0.60%)	⬆️
spatialdata/_core/_transform_elements.py	85.81% <100.00%> (-1.03%)	⬇️
spatialdata/_core/core_utils.py	92.39% <100.00%> (-2.93%)	⬇️
spatialdata/_io/read.py	97.46% <100.00%> (-0.20%)	⬇️
spatialdata/_io/write.py	96.05% <100.00%> (-0.23%)	⬇️
... and 1 more

[kevinyamauchi]

This LGTM! I think it makes sense to unify the Circles and polygons here. I left a couple of minor comments below.

[kevinyamauchi]

what are the 0, 3, and 6 here?

[giovp]

thse are defined by GeometryType enum by shapely and are used to set the geometry type in the parser.

[giovp]

@LucaMarconato thanks a lot for fixing tests!!! I think we can go on and merge this, one comment:

• I saw that pyarrow was re-added, do we need it? Think it's an artefact right?

[LucaMarconato]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

[giovp]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

wait, if it's for the cosmx reader then it should be in spatialdata io. There is no other way for the repr to not use pyarrow?

EDIT: wasn't needed to I've removed it again

EDIT EDIT: I was wrong, but do we really want to keep the pyarrow.read_table just for the repr?

[LucaMarconato]

@giovp yes for the moment better to keep it. Dask will allow to get the length of the dataframe soon (dask/dask#9973), at that point we can remove the dependency.

[ivirshup]

@LucaMarconato I don't think that dask is going to get this soon. I don't see an active PR for it at the moment, and it's been open for a few years now.

[giovp]

New Shapes model

This PR unifies polygons and shape elements in a single region element: Shapes.

Motivation

As of now, the Shapes element was stored as an AnnData, with a bunch of metadata to keep track of the type of shapes and size, and with the centroid coordinates store in in adata.obsm["spatial"]. Thinking about it more, it was very much unnecessary:

• since it's a region element, only minimal amount of information should be stored there, as any annotations would go into Tables -> because of this, the AnnData format is unneccesary
• it contained a lot of unnecessary metadata for determining which type of shapes are stored (circle, square only so far).
• it could be simplified by representing it as a geopandas dataframe.

Solution

Since a geopandas.GeoDataFrame is probably the better way to represent Circles and Squares shapes, instead of AnnData, it naturally follows that polygons and shapes should be grouped together. Benefits are the following:

• unified Shapes element (less complexity)
• less metadata, offload to shapely handling of different geometry types
• easier access to aggregation functions for Circles from geopandas.

e.g. example for counting points under circles regions @ivirshup

importgeopandasasgpdfromspatialdata._core.modelsimportShapesModelfromspatialdata._core.modelsimportPointsModelcircles=ShapesModel.parse(np.array([[10, 2], [5, 2]]), geometry=0, radius=[2, 3])
points=PointsModel.parse(
np.array([[11, 3], [5, 3]]), annotation=pd.DataFrame({"a": [0, 1]})
)
spot=circles["geometry"].buffer(distance=circles["radius"])[0]
gpd.points_from_xy(**points[["x", "y"]].compute()).within(spot)

Summary

Changes are minimal:

• The Polygons element (and models, IO, etc.) it's now called Shapes and it also contains Circles
• Circles are represented as a geopandas.GeoDataFrame of shapely.Point with additional column for radius
• The old Shapes element is removed
• No additional metadata is introduced, only change from IO side is read/write for radius column in the case of Circles

[giovp]

tests in transforms are failing but I'd wait for #100 to be merged first before fixing, as there will be many changes to be made as well.

[LucaMarconato]

@giovp all tests are passing now. I did just fixes, no real change. Anyway if you want to review I suggest to diff
70d3f16 (only merged the branches and ran the pre-commits)
with the latest commit.

[codecov]

Codecov Report

Merging #132 (3017790) into main (56179d6) will decrease coverage by 0.33%.
The diff coverage is 81.44%.

Additional details and impacted files

@@ Coverage Diff @@## main #132 +/- ##
==========================================
- Coverage 87.51% 87.19% -0.33% 
==========================================
Files 22 22 Lines 3108 3014 -94 ==========================================
- Hits 2720 2628 -92 + Misses 388 386 -2 

Impacted Files	Coverage Δ
spatialdata/__init__.py	100.00% <ø> (ø)
spatialdata/_io/__init__.py	100.00% <ø> (ø)
spatialdata/_core/_spatialdata.py	75.68% <60.00%> (-1.17%)	⬇️
spatialdata/_core/_spatial_query.py	77.21% <62.50%> (ø)
spatialdata/_core/models.py	85.83% <80.00%> (+0.04%)	⬆️
spatialdata/_io/format.py	88.23% <83.33%> (+0.60%)	⬆️
spatialdata/_core/_transform_elements.py	85.81% <100.00%> (-1.03%)	⬇️
spatialdata/_core/core_utils.py	92.39% <100.00%> (-2.93%)	⬇️
spatialdata/_io/read.py	97.46% <100.00%> (-0.20%)	⬇️
spatialdata/_io/write.py	96.05% <100.00%> (-0.23%)	⬇️
... and 1 more

[kevinyamauchi]

This LGTM! I think it makes sense to unify the Circles and polygons here. I left a couple of minor comments below.

[kevinyamauchi]

what are the 0, 3, and 6 here?

[giovp]

thse are defined by GeometryType enum by shapely and are used to set the geometry type in the parser.

[giovp]

@LucaMarconato thanks a lot for fixing tests!!! I think we can go on and merge this, one comment:

• I saw that pyarrow was re-added, do we need it? Think it's an artefact right?

[LucaMarconato]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

[giovp]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

wait, if it's for the cosmx reader then it should be in spatialdata io. There is no other way for the repr to not use pyarrow?

EDIT: wasn't needed to I've removed it again

EDIT EDIT: I was wrong, but do we really want to keep the pyarrow.read_table just for the repr?

[LucaMarconato]

@giovp yes for the moment better to keep it. Dask will allow to get the length of the dataframe soon (dask/dask#9973), at that point we can remove the dependency.

[ivirshup]

@LucaMarconato I don't think that dask is going to get this soon. I don't see an active PR for it at the moment, and it's been open for a few years now.

[giovp]

New Shapes model

This PR unifies polygons and shape elements in a single region element: Shapes.

Motivation

As of now, the Shapes element was stored as an AnnData, with a bunch of metadata to keep track of the type of shapes and size, and with the centroid coordinates store in in adata.obsm["spatial"]. Thinking about it more, it was very much unnecessary:

• since it's a region element, only minimal amount of information should be stored there, as any annotations would go into Tables -> because of this, the AnnData format is unneccesary
• it contained a lot of unnecessary metadata for determining which type of shapes are stored (circle, square only so far).
• it could be simplified by representing it as a geopandas dataframe.

Solution

Since a geopandas.GeoDataFrame is probably the better way to represent Circles and Squares shapes, instead of AnnData, it naturally follows that polygons and shapes should be grouped together. Benefits are the following:

• unified Shapes element (less complexity)
• less metadata, offload to shapely handling of different geometry types
• easier access to aggregation functions for Circles from geopandas.

e.g. example for counting points under circles regions @ivirshup

importgeopandasasgpdfromspatialdata._core.modelsimportShapesModelfromspatialdata._core.modelsimportPointsModelcircles=ShapesModel.parse(np.array([[10, 2], [5, 2]]), geometry=0, radius=[2, 3])
points=PointsModel.parse(
np.array([[11, 3], [5, 3]]), annotation=pd.DataFrame({"a": [0, 1]})
)
spot=circles["geometry"].buffer(distance=circles["radius"])[0]
gpd.points_from_xy(**points[["x", "y"]].compute()).within(spot)

Summary

Changes are minimal:

• The Polygons element (and models, IO, etc.) it's now called Shapes and it also contains Circles
• Circles are represented as a geopandas.GeoDataFrame of shapely.Point with additional column for radius
• The old Shapes element is removed
• No additional metadata is introduced, only change from IO side is read/write for radius column in the case of Circles

[giovp]

tests in transforms are failing but I'd wait for #100 to be merged first before fixing, as there will be many changes to be made as well.

[LucaMarconato]

@giovp all tests are passing now. I did just fixes, no real change. Anyway if you want to review I suggest to diff
70d3f16 (only merged the branches and ran the pre-commits)
with the latest commit.

[codecov]

Codecov Report

Merging #132 (3017790) into main (56179d6) will decrease coverage by 0.33%.
The diff coverage is 81.44%.

Additional details and impacted files

@@ Coverage Diff @@## main #132 +/- ##
==========================================
- Coverage 87.51% 87.19% -0.33% 
==========================================
Files 22 22 Lines 3108 3014 -94 ==========================================
- Hits 2720 2628 -92 + Misses 388 386 -2 

Impacted Files	Coverage Δ
spatialdata/__init__.py	100.00% <ø> (ø)
spatialdata/_io/__init__.py	100.00% <ø> (ø)
spatialdata/_core/_spatialdata.py	75.68% <60.00%> (-1.17%)	⬇️
spatialdata/_core/_spatial_query.py	77.21% <62.50%> (ø)
spatialdata/_core/models.py	85.83% <80.00%> (+0.04%)	⬆️
spatialdata/_io/format.py	88.23% <83.33%> (+0.60%)	⬆️
spatialdata/_core/_transform_elements.py	85.81% <100.00%> (-1.03%)	⬇️
spatialdata/_core/core_utils.py	92.39% <100.00%> (-2.93%)	⬇️
spatialdata/_io/read.py	97.46% <100.00%> (-0.20%)	⬇️
spatialdata/_io/write.py	96.05% <100.00%> (-0.23%)	⬇️
... and 1 more

[kevinyamauchi]

This LGTM! I think it makes sense to unify the Circles and polygons here. I left a couple of minor comments below.

[kevinyamauchi]

what are the 0, 3, and 6 here?

[giovp]

thse are defined by GeometryType enum by shapely and are used to set the geometry type in the parser.

[giovp]

@LucaMarconato thanks a lot for fixing tests!!! I think we can go on and merge this, one comment:

• I saw that pyarrow was re-added, do we need it? Think it's an artefact right?

[LucaMarconato]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

[giovp]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

wait, if it's for the cosmx reader then it should be in spatialdata io. There is no other way for the repr to not use pyarrow?

EDIT: wasn't needed to I've removed it again

EDIT EDIT: I was wrong, but do we really want to keep the pyarrow.read_table just for the repr?

[LucaMarconato]

@giovp yes for the moment better to keep it. Dask will allow to get the length of the dataframe soon (dask/dask#9973), at that point we can remove the dependency.

[ivirshup]

@LucaMarconato I don't think that dask is going to get this soon. I don't see an active PR for it at the moment, and it's been open for a few years now.

[giovp]

New Shapes model

This PR unifies polygons and shape elements in a single region element: Shapes.

Motivation

As of now, the Shapes element was stored as an AnnData, with a bunch of metadata to keep track of the type of shapes and size, and with the centroid coordinates store in in adata.obsm["spatial"]. Thinking about it more, it was very much unnecessary:

• since it's a region element, only minimal amount of information should be stored there, as any annotations would go into Tables -> because of this, the AnnData format is unneccesary
• it contained a lot of unnecessary metadata for determining which type of shapes are stored (circle, square only so far).
• it could be simplified by representing it as a geopandas dataframe.

Solution

Since a geopandas.GeoDataFrame is probably the better way to represent Circles and Squares shapes, instead of AnnData, it naturally follows that polygons and shapes should be grouped together. Benefits are the following:

• unified Shapes element (less complexity)
• less metadata, offload to shapely handling of different geometry types
• easier access to aggregation functions for Circles from geopandas.

e.g. example for counting points under circles regions @ivirshup

importgeopandasasgpdfromspatialdata._core.modelsimportShapesModelfromspatialdata._core.modelsimportPointsModelcircles=ShapesModel.parse(np.array([[10, 2], [5, 2]]), geometry=0, radius=[2, 3])
points=PointsModel.parse(
np.array([[11, 3], [5, 3]]), annotation=pd.DataFrame({"a": [0, 1]})
)
spot=circles["geometry"].buffer(distance=circles["radius"])[0]
gpd.points_from_xy(**points[["x", "y"]].compute()).within(spot)

Summary

Changes are minimal:

• The Polygons element (and models, IO, etc.) it's now called Shapes and it also contains Circles
• Circles are represented as a geopandas.GeoDataFrame of shapely.Point with additional column for radius
• The old Shapes element is removed
• No additional metadata is introduced, only change from IO side is read/write for radius column in the case of Circles

[giovp]

tests in transforms are failing but I'd wait for #100 to be merged first before fixing, as there will be many changes to be made as well.

[LucaMarconato]

@giovp all tests are passing now. I did just fixes, no real change. Anyway if you want to review I suggest to diff
70d3f16 (only merged the branches and ran the pre-commits)
with the latest commit.

[codecov]

Codecov Report

Merging #132 (3017790) into main (56179d6) will decrease coverage by 0.33%.
The diff coverage is 81.44%.

Additional details and impacted files

@@ Coverage Diff @@## main #132 +/- ##
==========================================
- Coverage 87.51% 87.19% -0.33% 
==========================================
Files 22 22 Lines 3108 3014 -94 ==========================================
- Hits 2720 2628 -92 + Misses 388 386 -2 

Impacted Files	Coverage Δ
spatialdata/__init__.py	100.00% <ø> (ø)
spatialdata/_io/__init__.py	100.00% <ø> (ø)
spatialdata/_core/_spatialdata.py	75.68% <60.00%> (-1.17%)	⬇️
spatialdata/_core/_spatial_query.py	77.21% <62.50%> (ø)
spatialdata/_core/models.py	85.83% <80.00%> (+0.04%)	⬆️
spatialdata/_io/format.py	88.23% <83.33%> (+0.60%)	⬆️
spatialdata/_core/_transform_elements.py	85.81% <100.00%> (-1.03%)	⬇️
spatialdata/_core/core_utils.py	92.39% <100.00%> (-2.93%)	⬇️
spatialdata/_io/read.py	97.46% <100.00%> (-0.20%)	⬇️
spatialdata/_io/write.py	96.05% <100.00%> (-0.23%)	⬇️
... and 1 more

[kevinyamauchi]

This LGTM! I think it makes sense to unify the Circles and polygons here. I left a couple of minor comments below.

[kevinyamauchi]

what are the 0, 3, and 6 here?

[giovp]

thse are defined by GeometryType enum by shapely and are used to set the geometry type in the parser.

[giovp]

@LucaMarconato thanks a lot for fixing tests!!! I think we can go on and merge this, one comment:

• I saw that pyarrow was re-added, do we need it? Think it's an artefact right?

[LucaMarconato]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

[giovp]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

wait, if it's for the cosmx reader then it should be in spatialdata io. There is no other way for the repr to not use pyarrow?

EDIT: wasn't needed to I've removed it again

EDIT EDIT: I was wrong, but do we really want to keep the pyarrow.read_table just for the repr?

[LucaMarconato]

@giovp yes for the moment better to keep it. Dask will allow to get the length of the dataframe soon (dask/dask#9973), at that point we can remove the dependency.

[ivirshup]

@LucaMarconato I don't think that dask is going to get this soon. I don't see an active PR for it at the moment, and it's been open for a few years now.

[giovp]

New Shapes model

This PR unifies polygons and shape elements in a single region element: Shapes.

Motivation

As of now, the Shapes element was stored as an AnnData, with a bunch of metadata to keep track of the type of shapes and size, and with the centroid coordinates store in in adata.obsm["spatial"]. Thinking about it more, it was very much unnecessary:

• since it's a region element, only minimal amount of information should be stored there, as any annotations would go into Tables -> because of this, the AnnData format is unneccesary
• it contained a lot of unnecessary metadata for determining which type of shapes are stored (circle, square only so far).
• it could be simplified by representing it as a geopandas dataframe.

Solution

Since a geopandas.GeoDataFrame is probably the better way to represent Circles and Squares shapes, instead of AnnData, it naturally follows that polygons and shapes should be grouped together. Benefits are the following:

• unified Shapes element (less complexity)
• less metadata, offload to shapely handling of different geometry types
• easier access to aggregation functions for Circles from geopandas.

e.g. example for counting points under circles regions @ivirshup

importgeopandasasgpdfromspatialdata._core.modelsimportShapesModelfromspatialdata._core.modelsimportPointsModelcircles=ShapesModel.parse(np.array([[10, 2], [5, 2]]), geometry=0, radius=[2, 3])
points=PointsModel.parse(
np.array([[11, 3], [5, 3]]), annotation=pd.DataFrame({"a": [0, 1]})
)
spot=circles["geometry"].buffer(distance=circles["radius"])[0]
gpd.points_from_xy(**points[["x", "y"]].compute()).within(spot)

Summary

Changes are minimal:

• The Polygons element (and models, IO, etc.) it's now called Shapes and it also contains Circles
• Circles are represented as a geopandas.GeoDataFrame of shapely.Point with additional column for radius
• The old Shapes element is removed
• No additional metadata is introduced, only change from IO side is read/write for radius column in the case of Circles

[giovp]

tests in transforms are failing but I'd wait for #100 to be merged first before fixing, as there will be many changes to be made as well.

[LucaMarconato]

@giovp all tests are passing now. I did just fixes, no real change. Anyway if you want to review I suggest to diff
70d3f16 (only merged the branches and ran the pre-commits)
with the latest commit.

[codecov]

Codecov Report

Merging #132 (3017790) into main (56179d6) will decrease coverage by 0.33%.
The diff coverage is 81.44%.

Additional details and impacted files

@@ Coverage Diff @@## main #132 +/- ##
==========================================
- Coverage 87.51% 87.19% -0.33% 
==========================================
Files 22 22 Lines 3108 3014 -94 ==========================================
- Hits 2720 2628 -92 + Misses 388 386 -2 

Impacted Files	Coverage Δ
spatialdata/__init__.py	100.00% <ø> (ø)
spatialdata/_io/__init__.py	100.00% <ø> (ø)
spatialdata/_core/_spatialdata.py	75.68% <60.00%> (-1.17%)	⬇️
spatialdata/_core/_spatial_query.py	77.21% <62.50%> (ø)
spatialdata/_core/models.py	85.83% <80.00%> (+0.04%)	⬆️
spatialdata/_io/format.py	88.23% <83.33%> (+0.60%)	⬆️
spatialdata/_core/_transform_elements.py	85.81% <100.00%> (-1.03%)	⬇️
spatialdata/_core/core_utils.py	92.39% <100.00%> (-2.93%)	⬇️
spatialdata/_io/read.py	97.46% <100.00%> (-0.20%)	⬇️
spatialdata/_io/write.py	96.05% <100.00%> (-0.23%)	⬇️
... and 1 more

[kevinyamauchi]

This LGTM! I think it makes sense to unify the Circles and polygons here. I left a couple of minor comments below.

[kevinyamauchi]

what are the 0, 3, and 6 here?

[giovp]

thse are defined by GeometryType enum by shapely and are used to set the geometry type in the parser.

[giovp]

@LucaMarconato thanks a lot for fixing tests!!! I think we can go on and merge this, one comment:

• I saw that pyarrow was re-added, do we need it? Think it's an artefact right?

[LucaMarconato]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

[giovp]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

wait, if it's for the cosmx reader then it should be in spatialdata io. There is no other way for the repr to not use pyarrow?

EDIT: wasn't needed to I've removed it again

EDIT EDIT: I was wrong, but do we really want to keep the pyarrow.read_table just for the repr?

[LucaMarconato]

@giovp yes for the moment better to keep it. Dask will allow to get the length of the dataframe soon (dask/dask#9973), at that point we can remove the dependency.

[ivirshup]

@LucaMarconato I don't think that dask is going to get this soon. I don't see an active PR for it at the moment, and it's been open for a few years now.

[giovp]

New Shapes model

This PR unifies polygons and shape elements in a single region element: Shapes.

Motivation

As of now, the Shapes element was stored as an AnnData, with a bunch of metadata to keep track of the type of shapes and size, and with the centroid coordinates store in in adata.obsm["spatial"]. Thinking about it more, it was very much unnecessary:

• since it's a region element, only minimal amount of information should be stored there, as any annotations would go into Tables -> because of this, the AnnData format is unneccesary
• it contained a lot of unnecessary metadata for determining which type of shapes are stored (circle, square only so far).
• it could be simplified by representing it as a geopandas dataframe.

Solution

Since a geopandas.GeoDataFrame is probably the better way to represent Circles and Squares shapes, instead of AnnData, it naturally follows that polygons and shapes should be grouped together. Benefits are the following:

• unified Shapes element (less complexity)
• less metadata, offload to shapely handling of different geometry types
• easier access to aggregation functions for Circles from geopandas.

e.g. example for counting points under circles regions @ivirshup

importgeopandasasgpdfromspatialdata._core.modelsimportShapesModelfromspatialdata._core.modelsimportPointsModelcircles=ShapesModel.parse(np.array([[10, 2], [5, 2]]), geometry=0, radius=[2, 3])
points=PointsModel.parse(
np.array([[11, 3], [5, 3]]), annotation=pd.DataFrame({"a": [0, 1]})
)
spot=circles["geometry"].buffer(distance=circles["radius"])[0]
gpd.points_from_xy(**points[["x", "y"]].compute()).within(spot)

Summary

Changes are minimal:

• The Polygons element (and models, IO, etc.) it's now called Shapes and it also contains Circles
• Circles are represented as a geopandas.GeoDataFrame of shapely.Point with additional column for radius
• The old Shapes element is removed
• No additional metadata is introduced, only change from IO side is read/write for radius column in the case of Circles

[giovp]

tests in transforms are failing but I'd wait for #100 to be merged first before fixing, as there will be many changes to be made as well.

[LucaMarconato]

@giovp all tests are passing now. I did just fixes, no real change. Anyway if you want to review I suggest to diff
70d3f16 (only merged the branches and ran the pre-commits)
with the latest commit.

[codecov]

Codecov Report

Merging #132 (3017790) into main (56179d6) will decrease coverage by 0.33%.
The diff coverage is 81.44%.

Additional details and impacted files

@@ Coverage Diff @@## main #132 +/- ##
==========================================
- Coverage 87.51% 87.19% -0.33% 
==========================================
Files 22 22 Lines 3108 3014 -94 ==========================================
- Hits 2720 2628 -92 + Misses 388 386 -2 

Impacted Files	Coverage Δ
spatialdata/__init__.py	100.00% <ø> (ø)
spatialdata/_io/__init__.py	100.00% <ø> (ø)
spatialdata/_core/_spatialdata.py	75.68% <60.00%> (-1.17%)	⬇️
spatialdata/_core/_spatial_query.py	77.21% <62.50%> (ø)
spatialdata/_core/models.py	85.83% <80.00%> (+0.04%)	⬆️
spatialdata/_io/format.py	88.23% <83.33%> (+0.60%)	⬆️
spatialdata/_core/_transform_elements.py	85.81% <100.00%> (-1.03%)	⬇️
spatialdata/_core/core_utils.py	92.39% <100.00%> (-2.93%)	⬇️
spatialdata/_io/read.py	97.46% <100.00%> (-0.20%)	⬇️
spatialdata/_io/write.py	96.05% <100.00%> (-0.23%)	⬇️
... and 1 more

[kevinyamauchi]

This LGTM! I think it makes sense to unify the Circles and polygons here. I left a couple of minor comments below.

[kevinyamauchi]

what are the 0, 3, and 6 here?

[giovp]

thse are defined by GeometryType enum by shapely and are used to set the geometry type in the parser.

[giovp]

@LucaMarconato thanks a lot for fixing tests!!! I think we can go on and merge this, one comment:

• I saw that pyarrow was re-added, do we need it? Think it's an artefact right?

[LucaMarconato]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

[giovp]

Thanks for the PR 😊 we need it because it's used in the PR fixing the __repr__ for SpatialData (also, I noticed that we need it in SpatialData io for thr cosmx reader)

wait, if it's for the cosmx reader then it should be in spatialdata io. There is no other way for the repr to not use pyarrow?

EDIT: wasn't needed to I've removed it again

EDIT EDIT: I was wrong, but do we really want to keep the pyarrow.read_table just for the repr?

[LucaMarconato]

@giovp yes for the moment better to keep it. Dask will allow to get the length of the dataframe soon (dask/dask#9973), at that point we can remove the dependency.

[ivirshup]

@LucaMarconato I don't think that dask is going to get this soon. I don't see an active PR for it at the moment, and it's been open for a few years now.