-
Notifications
You must be signed in to change notification settings - Fork 16
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
Optical flow parallelization #73
base: master
Are you sure you want to change the base?
Optical flow parallelization #73
Conversation
@@ -59,7 +59,7 @@ use rule template as optical_flow with: | |||
script = SCRIPTS / 'optical_flow.py' | |||
params: | |||
params('alpha', 'max_Niter', 'convergence_limit', 'gaussian_sigma', | |||
'derivative_filter', 'use_phases', config=config) | |||
'derivative_filter', 'use_phases', 'n_jobs', 'max_padding_iterations', config=config) |
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.
These parameters would also need to be added to the config_template.yaml file.
I think the number of jobs isn't really a parameter that should be set by the user, but rather set automatically by inferring the available resources, for example, by using multiprocessing.cpu_count()
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.
Ok we will modify the config_template.yaml file coherently.
It may depend on the specific user and platform.
In addition, multiprocessing.cpu_count()
does not differentiate between physical and logical cores. Thus allowing the optical_flow.py script to use all the output of multiprocessing.cpu_count()
may not result into computational advantage and may interfere with other processes running.
For this reason we suggest the usage of psutil library and a ratio of psutil.cpu_count(logical = False)
output to set the number of parallel processes based on the number of physical cores, in case the user does not set explicitly the n_jobs parameter (which can be set to None as default).
for i in tqdm(range(len(frames[:-1])), ascii=True)) | ||
|
||
vector_frames = np.asarray(vector_frames, dtype=complex) | ||
vector_frames[:,nan_channels[0],nan_channels[1]] = np.nan + np.nan*1j |
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 formulation does not generalize to possible 1D frames
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.
If the 1D case is represented as a vector-like matrix of shape (dim_x, 1), this notation would still be consistent.
Otherwise if the shape would just be (dim_x), then the script would break here and elsewhere in the code, e.g.
- in many parts of the code we explicitly extract the shape:
dim_y, dim_x = frames[0].shape
- in plot_opticalflow function we assume 2D arrays and explicitly extract the shape:
dim_y, dim_x = vec_frame.shape
And could be not properly defined the algorithm when we compute the derivatives on both the x and y axes:
fx = phase_conv2d(frame, kernelX) \
+ phase_conv2d(next_frame, kernelX)
fy = phase_conv2d(frame, kernelY) \
+ phase_conv2d(next_frame, kernelY)
return frames | ||
else: | ||
grid_y, grid_x = np.meshgrid([-1, 0, 1], [-1, 0, 1], indexing='ij') | ||
print('Frames interpolation') |
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.
remove print statement. instead you could set this as a title for tqdm
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.
Ok, we will add a title to the tqdm progress bar.
grid_x=grid_x, | ||
are_phases=are_phases, | ||
max_iters=max_iters) | ||
for i in tqdm(range(len(frames)), ascii=True)) |
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.
tqdm would be a new dependency of cobrawap and would need to be added to the environment and pyproject definition
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.
Yes, we will update the environment file and the pyproject accordingly.
Introduction of joblib library to parallelize the execution of interpolate_empty_sites and horn_schunck functions over the different time frames.
Speed up time depends on machine possibility to parallelize multiple processes over the cores.
WARNING: This implementation also fixes the vector_frames size issue taken into account by PR #71.
Please consider to merge PR #71 before this one for result consistency.