Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix query of 2D/3D data with 2D/3D bounding box #409
Fix query of 2D/3D data with 2D/3D bounding box #409
Changes from 9 commits
7936f48
a41d3d1
af214b3
386b6d8
65516af
186be02
4e64e16
255bd4e
82b6061
eecc0b9
bedbc80
d0ff8fc
452cbc7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I indeed also worry that the solution is rather complicated, what does it mean that "z and x,y are mixed" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.