DM-31839: Reduce memory usage in MatchPessemisticB #148
Conversation
There was a problem hiding this comment.
This looks good to me (some minor comments). Could I ask you to be a bit less opaque in the commit message? I.e.,
Change temp id array to uint16 --> WHY (AND FROM WHAT)?
Change to unit16 --> HOW IS THIS DISTINCT FROM ABOVE?
Square root dists. --> IS THIS TO IMPLY YOU WEREN'T DOING THIS BEFORE?
Fix linting. --> HIGHLIGHTS THAT THIS SHOULD'VE BEEN DONE ON A SEPARATE COMMIT (but I'm not too fussed about this one 😉)
Fix math bug. --> PLEASE ELABORATE (IF THERE WAS A BUG THAT GOT FIXED, HOW ARE THE PRE AND POST RESULTS IDENTICAL AS YOU STATE ON THE TICKET?)
| tmp_ref_dist_array = np.sqrt( | ||
| ((self._reference_array | ||
| - self._reference_array[ref_id]) | ||
| ** 2).sum(axis=1)).astype("float32") |
There was a problem hiding this comment.
You seem to mix between using
dtype=np.uint16 above vs. dtype="uint16" here and
astype("float32") here vs. astype(np.float32) above.
There was a problem hiding this comment.
I'll change them to be consistent.
| @@ -95,15 +95,7 @@ def _build_distances_and_angles(self): | |||
|
|
|||
| # Initialize the arrays we will need for quick look up of pairs once | |||
| # have a candidate spoke center. | |||
| self._pair_id_array = np.empty( | |||
There was a problem hiding this comment.
Should the above comment now be removed?
| dtype=np.float32) | ||
|
|
||
| # Create empty lists to temporarily store our pair information per | ||
| # reference object. These will be concatenated into our final arrays. |
There was a problem hiding this comment.
Is this comment actually still relevant?
| tmp_ref_dist_array = np.sqrt( | ||
| ((self._reference_array | ||
| - self._reference_array[ref_id]) | ||
| ** 2).sum(axis=1)).astype("float32") |
There was a problem hiding this comment.
Can you help me understand what the sum(axis=1) is doing here (not saying it's wrong, I just don't know what's going on...oh, is it a sum over the squares of the deltas in all three axes which was done more "spelled out" for sub_dist_array above?)?
There was a problem hiding this comment.
sum without an axis argument sums the full array. sum(axis=1) sums only along the second axis. These are 3 vector distances so this is basically a dot product.
|
I'll just remove those extra comments from the commit. They are left overs from developing, debugging, and committing at the same time. |
Simplify precomputation of distances.
Remove/add comments. Consistently invoke types.
morriscb
force-pushed
the
tickets/DM-31839
branch
from
8320b89
to
bf906a7
Compare
No description provided.