-
Notifications
You must be signed in to change notification settings - Fork 45
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
vectorize bounding box query #699
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #699 +/- ##
==========================================
- Coverage 91.83% 91.76% -0.07%
==========================================
Files 44 45 +1
Lines 6781 6887 +106
==========================================
+ Hits 6227 6320 +93
- Misses 554 567 +13
|
ready for a first pass of review |
ready for review |
Thank you @giovp for the PR, I will review now.
I can't think of other places right now, so I think we are good with the current improvement.
I agree. |
|
||
@nb.njit(parallel=False, nopython=True) | ||
def _create_slices_and_translation( | ||
min_values: nb.types.Array[nb.float64, nb.float64], |
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 think nb.types.Array[nb.float64, np.float64]
may be incorrect and that the correct version is nb.types.Array(nb.float64, 2, 'C')
. But I am not sure because pre-commit doesn't complain, so maybe both syntaxes are allowed.
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.
Mine is wrong. Are you sure about your typing? What looks strange to me is that types like nb.types.Array[nb.float64, nb.int64]
would not have a meaning. I tried using just nb.types.Array
and pre-commit works, maybe this is the way to go.
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 will look into it!
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 removed the dtype, I'd merge.
@giovp I finished reviewing; I applied some minor changes like simplified the docs and added an extra test. |
Trying to vectorize the tiling in the dataloader PR, realized some improvements should be added separately.
With this PR, I enable the vectorization of
bounding_box_query
for all elements.This means that it is now possible to pass an array of bounding boxes (and not just one).
TODO:
@LucaMarconato @melonora do you have any suggestion of when this could be used to replace current implementations across the projects? A clear use case (and the motivation for this contribution) is the dataloader, see #687 for the speedup, but I wonder if there are other places where this is useful.
This also is the groundwork for eventual update to the dataloader, being able to return batches of all elements.
considering that this PR is already getting too large, I would postpone the vectorization of polygon query.