Refactor polygon_query()
#422
Conversation
for more information, see https://pre-commit.ci
…ialdata into fix_query_3d_points
for more information, see https://pre-commit.ci
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
==========================================
+ Coverage 91.78% 92.13% +0.34%
==========================================
Files 37 37
Lines 5052 5073 +21
==========================================
+ Hits 4637 4674 +37
+ Misses 415 399 -16
|
| axes_only_in_bb = set(axes) - set(output_axes_without_c) | ||
| axes_only_in_output = set(output_axes_without_c) - set(axes) | ||
|
|
||
| # let's remove from the bounding box whose axes that are not in the output axes (e.g. querying 2D points with a | ||
| # 3D bounding box) | ||
| indices_to_remove_from_bb = [axes.index(ax) for ax in axes_only_in_bb] | ||
| axes = tuple([ax for ax in axes if ax not in axes_only_in_bb]) | ||
| min_coordinate = np.delete(min_coordinate, indices_to_remove_from_bb) | ||
| max_coordinate = np.delete(max_coordinate, indices_to_remove_from_bb) |
There was a problem hiding this comment.
min_coordinate and max_coordinate are with respect to axes right? If so
idx_bb = np.in1d(axes, output_axes_without_c)
idx_out = np.in1d(output_axes_without_c, axes)
min_coordinate = min_coordinate[idx_out]
max_coordinate = max_coordinate[idx_out]There was a problem hiding this comment.
if yes, I would change axes to axes_bb and maybe output_axes_without_c to axes_out_without_x to anyway make them consistent
There was a problem hiding this comment.
also, do you need to check (maybe done outside) that min/max coordinate matches in len with some axes (either bb or out)
There was a problem hiding this comment.
min_coordinate and max_coordinate are with respect to axes right? If so
idx_bb = np.in1d(axes, output_axes_without_c)
idx_out = np.in1d(output_axes_without_c, axes)
this still returns a boolean mask, so it's basically equivalent to what I wrote; I'll keep the old code.
min_coordinate = min_coordinate[idx_out]
max_coordinate = max_coordinate[idx_out]
There was a problem hiding this comment.
if yes, I would change axes to axes_bb and maybe output_axes_without_c to axes_out_without_x to anyway make them consistent
good idea, I have renamed these two variables
There was a problem hiding this comment.
yes, we check that it matches with bb, and we do it BoundingBoxRequest.__post_init__()
There was a problem hiding this comment.
Thanks, @LucaMarconato . I think this looks pretty good. I've left some comments below, mostly related to code clarity.
I am okay with the refactors in test_spatial_query. I see that they likely reduce the amount of code. However, I think this is at the cost of being able to quickly understand the intention of the test. If that's the trade-off we want, I am fine with it. I would add some comments and docstrings to make it easier to understand though.
| Please see | ||
| :func:`spatialdata.bounding_box_query` for the complete docstring. |
There was a problem hiding this comment.
what is the reason for this change? so you don't have to maintain two docstrings? I think it's fine, but I'm curious
There was a problem hiding this comment.
I see you did the same below for polygon. I would keep in mind that this is going to be a really common way that users interact with the query interface. With this change, I don't think they will have access to the docstring when they are using an IDE (e.g., inspecting sdata.query.polygon()`
There was a problem hiding this comment.
Exactly, I would like to have it only in one place because it's easy to forget to update the other (happened). Is there any Sphinx trick we can use here? Asking also @giovp. For this PR I'll keep it like this because I'd like to merge some other PRs dependent on this.
There was a problem hiding this comment.
I see also torch has something similar to what is in this PR: https://pytorch.org/docs/stable/generated/torch.Tensor.sum.html
Made on top of #409, which should be merged first.
Closes #255.
The PR does the following:
polygon_queryandbounding_box_queryAPIs. In particularpolygon_query()sdata.query.polygon_querypolygon_querycompletepolygon_queryandbounding_box_querywithin the same testMinor:
PointsModel.validate()(which is also called by the parser) now checks that no radius value is <= 0. Added also a test for this.