Fix query of 2D/3D data with 2D/3D bounding box

[LucaMarconato]

I improved the 3D support for bounding box queries to cover the following cases:

• points: data can be 2D/3D, bounding box can be 2D/3D
• images and labels: data can be 2D/3D, bounding box can be 2D/3D

I added tests to all the above, and in doing this I refactored the old tests for raster data to use pytest.mark.parameterize().

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (9470a61) 91.60% compared to head (452cbc7) 91.68%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #409      +/-   ##
==========================================
+ Coverage   91.60%   91.68%   +0.07%     
==========================================
  Files          37       37              
  Lines        5017     5053      +36     
==========================================
+ Hits         4596     4633      +37     
+ Misses        421      420       -1     

Files	Coverage Δ
src/spatialdata/_core/query/spatial_query.py	92.92% <96.96%> (+0.90%)	⬆️

... and 1 file with indirect coverage changes

[kevinyamauchi]

I don't have a ton of time to review in depth right now, but I had a quick look. I think these are good bugs to address, but I don't think I fully understand the solution.

What is happening when there is a 2D bounding box and 3D data?

If somebody else has time to review and approves, please feel free to merge without me.

[kevinyamauchi]

Is this the best approach? I'm fine if this is a temporary solution to move us forward, but it feels a bit odd. Maybe just add a comment suggesting what we could do in the future?

[LucaMarconato]

I am working on a follow up PR to fix problems with polygon_query, in that context I have found what I think is a better approach. In that cases one has a (2D) polygon to query 3D points. What I suggest is to restrict to the case in which the transformation is invertible (as I do for the bounding_box_query for raster data) and then compute the counter image of the polygon in the intrinsic coordinate system. This polygon will lie on a plane. The better approach says to project the 3D points on this plane and then perform a 2D query.

Anyway, also in that case I haven't implemented the full approach, rather I check that the z coordinate is not being mixed with x and y coordinates, so that the plane is assured to be orthogonal to the z axis in the intrinsic coordinate system. Here is the code:

spatialdata/src/spatialdata/_core/query/spatial_query.py

Line 122 in aeb2db0

# we need to deal with the equivalent of _adjust_bounding_box_to_real_axes() to account for the fact that the

[LucaMarconato]

The approach used here with -M, M is a bit more general because it allows for the case in which z and x, y are mixed (but in practice probably this is useless and it just complicates things). Also the bounding_box query for points doesn't require the transformation to be invertible (again, in practice probably this is useless, and it's also slower).

I propose to add all these things as comments, and then in a future follow up PR address these things and clear the code a bit. But for the moment I think that this fix and the fixes on polygon_queries have higher priority.

[giovp]

Anyway, also in that case I haven't implemented the full approach, rather I check that the z coordinate is not being mixed with x and y coordinates, so that the plane is assured to be orthogonal to the z axis in the intrinsic coordinate system.
The approach used here with -M, M is a bit more general because it allows for the case in which z and x, y are mixed (but in practice probably this is useless and it just complicates things).

I indeed also worry that the solution is rather complicated, what does it mean that "z and x,y are mixed" ?

[LucaMarconato]

Apologies it was not clear from my message, I meant when z depends on x and y, for instance in a non-invertible shear transformation.

[LucaMarconato]

The new implementation is in the middle between a general implementation and making some reasonable assumptions. I would keep it like this at this stage to move things forward and then refine in a follow up PR to simplify the code and cut some edge cases.

[LucaMarconato]

Thanks for the comments! The function _adjust_bounding_box_to_real_axes() is responsible to deal with the case in which the transformed data is 2D and the bounding box is specified to be 3D, and the case in which the transformed data is 3D and the bounding box is 2D.

[LucaMarconato]

If somebody else has time to review and approves, please feel free to merge without me.

@giovp @melonora would you have some time?

[giovp]

looks good @LucaMarconato ! I would really recc to leave a comment on the _adjust_bounding_box_to_real_axes because the behaviour is not very intuitive.

[giovp]

Anyway, also in that case I haven't implemented the full approach, rather I check that the z coordinate is not being mixed with x and y coordinates, so that the plane is assured to be orthogonal to the z axis in the intrinsic coordinate system.
The approach used here with -M, M is a bit more general because it allows for the case in which z and x, y are mixed (but in practice probably this is useless and it just complicates things).

I indeed also worry that the solution is rather complicated, what does it mean that "z and x,y are mixed" ?

[LucaMarconato]

@giovp @kevinyamauchi thanks for the review, I have added more comments in the code and now we are ready to merge.