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

get_extent() refactoring #318

Merged
merged 51 commits into from
Oct 10, 2023
Merged

get_extent() refactoring #318

merged 51 commits into from
Oct 10, 2023

Conversation

LucaMarconato
Copy link
Member

@LucaMarconato LucaMarconato commented Jul 11, 2023

Starting a PR to rework and move the get_extent() function from @timtreis from spatialdata-plot to spatialdata. This PR is needed for some tiles generation code that @EliHei2 is working on.

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #318 (13da8cd) into main (14bf585) will increase coverage by 0.67%.
The diff coverage is 97.20%.

❗ Current head 13da8cd differs from pull request most recent head d6bd822. Consider uploading reports for the commit d6bd822 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #318      +/-   ##
==========================================
+ Coverage   90.61%   91.28%   +0.67%     
==========================================
  Files          36       36              
  Lines        4700     4845     +145     
==========================================
+ Hits         4259     4423     +164     
+ Misses        441      422      -19     
Files Coverage Δ
src/spatialdata/__init__.py 100.00% <100.00%> (ø)
src/spatialdata/_core/operations/rasterize.py 83.41% <100.00%> (ø)
src/spatialdata/_core/query/spatial_query.py 92.01% <100.00%> (+0.08%) ⬆️
src/spatialdata/_types.py 66.66% <100.00%> (+4.16%) ⬆️
src/spatialdata/datasets.py 100.00% <100.00%> (ø)
src/spatialdata/models/_utils.py 89.89% <100.00%> (-0.11%) ⬇️
src/spatialdata/_core/query/_utils.py 92.00% <92.85%> (+1.09%) ⬆️
src/spatialdata/_core/data_extent.py 97.33% <97.27%> (+97.33%) ⬆️

... and 1 file with indirect coverage changes

@LucaMarconato LucaMarconato marked this pull request as ready for review July 11, 2023 15:22
@LucaMarconato
Copy link
Member Author

LucaMarconato commented Jul 11, 2023

Actually it's completed and ready for review. Talked with @timtreis and he may add some extra functionalities/convenience parameters (used in spatialdata-plot) in a follow up PR.

@LucaMarconato LucaMarconato linked an issue Aug 15, 2023 that may be closed by this pull request
@LucaMarconato
Copy link
Member Author

LucaMarconato commented Aug 26, 2023

@Sonja-Stockhaus thanks for the bugfix and the refactoring! Please let me know when you finish with the additions and the PR is ready for review. You can tag me here/write me in Zulip.

@Sonja-Stockhaus Sonja-Stockhaus linked an issue Aug 29, 2023 that may be closed by this pull request
# remove potentially empty geometries
e_temp = e[e["geometry"].apply(lambda geom: not geom.is_empty)]

# separate points from (multi-)polygons
Copy link
Member Author

@LucaMarconato LucaMarconato Oct 9, 2023

Choose a reason for hiding this comment

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

This is not needed because we assume that the GeoDataFrame doesn't mix Point with Polygon/MultiPolygon types. I'll removed it to simplify the code.

Copy link
Member

Choose a reason for hiding this comment

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

Very similar to a bugfix @Sonja-Stockhaus implemented in scverse/spatialdata-plot#133. Don't know if I'd delete this

Copy link
Member Author

Choose a reason for hiding this comment

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

In the PR I kept the removal of empty points, but I have removed the split of shapes and points. It can be easily restored inside the function _get_extent_of_shapes() if you want. We don't expect to need it now, but it would be still ok to have, just to make the code more future proof.

@LucaMarconato
Copy link
Member Author

I applied the code review from Giovanni and added a test for the xy swap bug: #335.

@LucaMarconato
Copy link
Member Author

As you can see, the Python 3.9 tests fail in a0e26ba (#318), but they succeed in 184dd82 (#318). Not sure why it happens, but as a workaround @timtreis suggested to replace the | with Union, and to silence Ruff.

@LucaMarconato LucaMarconato merged commit cf925c5 into main Oct 10, 2023
5 checks passed
@LucaMarconato LucaMarconato deleted the feature/get_extent branch October 10, 2023 15:49
@giovp giovp mentioned this pull request Nov 27, 2023
6 tasks
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.

get_extent switches x and y Refactor get_extent() from spatialdata-plot and push upstream
4 participants