-
Notifications
You must be signed in to change notification settings - Fork 26
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
Remove unnecessary variables, clean up warp_image #625
Conversation
gvec_angs = np.vstack([ | ||
self.angular_grid[1].flatten(), | ||
self.angular_grid[0].flatten(), |
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.
gvec_angs = np.vstack([ | |
self.angular_grid[1].flatten(), | |
self.angular_grid[0].flatten(), | |
angular_grid = self.angular_grid | |
gvec_angs = np.vstack([ | |
angular_grid[1].flatten(), | |
angular_grid[0].flatten(), |
Since self.angular_grid
is a computed property, you will probably get better performance by adding back the variable so it is only computed once.
@@ -233,42 +233,35 @@ def warp_image(self, image_dict, pad_with_nans=False, | |||
|
|||
""" | |||
|
|||
angpts = self.angular_grid | |||
dummy_ome = np.zeros((self.ntth*self.neta)) | |||
gvec_angs = np.vstack([ |
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.
Moving gvec_angs
outside the loop is a great idea here.
@@ -233,42 +233,35 @@ def warp_image(self, image_dict, pad_with_nans=False, | |||
|
|||
""" | |||
|
|||
angpts = self.angular_grid | |||
dummy_ome = np.zeros((self.ntth*self.neta)) |
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.
Removing unnecessary variables isn't always helpful, because they can help with readability (in this case, dummy_ome
indicates that we are making dummy omega values).
xypts = np.nan*np.ones((len(gvec_angs), 2)) | ||
valid_xys, rmats_s, on_plane = _project_on_detector(*args, | ||
**kwargs) | ||
valid_xys, _, on_plane = _project_on_detector(*args, **kwargs) |
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.
Could we just pass valid_xys
into the interpolation function directly, rather than setting xypts[on_plane, :] = valid_xys
below and passing xypts
? It appears that the interpolation functions ignore invalid xys, and only operate on a subset of the valid xys (in their functions, they check which xys are on the panel, and ignore xys not on the panel, and they also apply the panel buffer mask to remove even more xys).
Can you verify that you get the same result that way? It might be faster also...
More thoughts: the interpolation functions call clip_to_panel(xy, buffer_edges=True)
, which consists of two parts:
~~ 1. Removing any xys
that are not on the panel ~~
~~ 2. Applying the panel buffer mask to remove any masked xys
Since we already know that valid_xys
are on the panel, it would be nice if we could just skip the check for whether the xys
are on the panel. But we still need to apply the panel buffer mask (it hasn't been applied yet). Maybe we should make a separate way to apply the panel buffer mask so we can skip the extra check that clip_to_panel()
performs... but only if we verify that the result stays the same.
Correction: it looks like the original valid_xys
are on the detector plane, but not necessarily on the detector itself. So we do actually need to keep the clip_to_panel()
call.
However, I think it still might be better to just pass the valid_xys
into the interpolation function directly, since we don't need to pass in any xys that are not valid (i. e., nan
).
Cleaned up some of the logic in
warp_image
directly. Expected speedup ~4-5%