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
|
giovp
left a comment
giovp
left a comment
There was a problem hiding this comment.
small things around documentation
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__()
kevinyamauchi
left a comment
kevinyamauchi
left a comment
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
3 participants
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.