-
Notifications
You must be signed in to change notification settings - Fork 14
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
Batch search inside stack search #530
Batch search inside stack search #530
Conversation
… setup/clear resources
removed accidental git conflict merge conflicts dirac-institute#2
9e322bb
to
0b25c32
Compare
…so as to be able to init gpu_search_list outside of the main search func
@@ -0,0 +1,40 @@ | |||
class BatchSearchManager: |
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 main idea here was that I didn't want the user to have to call prepare_batch_search
and finish_search
manually
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.
Not for this PR, but something to consider as a future extension: In theory this could be extended further by making this a generator:
for batch_search in BatchSearchManager(stack_search, search_list, min_observations, x_size, y_size):
batch_results.extend(batch_search.search_batch())
where the BatchSearchManager
tracks the block sizes to search, current block locations, etc.
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.
A bunch of points for discussion.
@@ -0,0 +1,40 @@ | |||
class BatchSearchManager: |
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.
Not for this PR, but something to consider as a future extension: In theory this could be extended further by making this a generator:
for batch_search in BatchSearchManager(stack_search, search_list, min_observations, x_size, y_size):
batch_results.extend(batch_search.search_batch())
where the BatchSearchManager
tracks the block sizes to search, current block locations, etc.
…e_batch + removed move assignment operator in favour of set_trajectories + updated search to search_all across the python code
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
This is looking really good. A few stylistic cleanups.
src/kbmod/search/stack_search.cpp
Outdated
} | ||
|
||
|
||
int StackSearch::extract_max_results(){ |
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.
Good call breaking this out into a function.
tests/test_search.py
Outdated
stack = ImageStack(imlist) | ||
im_list = stack.get_images() | ||
# Create a new list of LayeredImages with the added object. | ||
new_im_list = [] |
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.
You can remove new_im_list
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.
Removed now
src/kbmod/search/stack_search.h
Outdated
@@ -29,6 +29,7 @@ using Point = indexing::Point; | |||
using Image = search::Image; | |||
|
|||
class StackSearch { | |||
int extract_max_results(); |
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.
Maybe rename this to get_max_results
or compute_max_results
to avoid any confusion about this somehow extracting the results from the cached list.
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.
Good point - renamed
src/kbmod/search/stack_search.cpp
Outdated
|
||
// Do the actual search on the GPU. | ||
DebugTimer search_timer = DebugTimer("search execution", rs_logger); | ||
// Do the actual search on the GPU. |
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.
Indentation of this comment appears off.
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.
Cleaned
This tackles #356
The idea is that you'd replace the full in-memory search:
With a batched version:
The steps are:
BatchSearchManager
context manager to obtain a managedbatch_search
object