DM-15046: When comparing angles in MatchPessimisticB, make sure all vectors are plane projected #94
Conversation
Remove unnecessary inputs from method call signatures. Fix call to _test_spoke
| test_rot_struct.shift_matrix is None: | ||
| continue | ||
|
|
||
| # Get the data from the return struct. | ||
| cos_rot_sq = test_rot_struct.cos_rot_sq | ||
| proj_ref_ctr_delta = test_rot_struct.proj_ref_ctr_delta |
There was a problem hiding this comment.
This name is not very clear compared to its description: "Plane projected 3 vector of the first candidate pair of the pinwheel." Is ctr meant to be center? Consider something starting with vector_proj.
There was a problem hiding this comment.
I've changed the description and hopefully that improves things.
| n_match) | ||
| self._reference_array[ref_id], ref_id, proj_ref_ctr_delta, | ||
| tmp_ref_delta_array, tmp_ref_dist_arary, | ||
| tmp_ref_id_array, max_dist_rad, n_match) |
There was a problem hiding this comment.
Typo: tmp_ref_dist_arary -> tmp_ref_dist_array
| np.dot(ref_delta, ref_center) * ref_center) | ||
| cos_rot_sq = (np.dot(proj_src_delta, proj_ref_delta) ** 2 / | ||
| (np.dot(proj_src_delta, proj_src_delta) * | ||
| np.dot(proj_ref_delta, proj_ref_delta))) |
There was a problem hiding this comment.
I assume that the dot products of vectors with themselves are meant for clarity but you could do np.sum(np.square(x)) instead.
There was a problem hiding this comment.
Correct, the dots are for clarity and simplicity. I can't say I'm really for making the change as it changes one call into two. I'll leave it as is.
| np.dot(src_delta_array[src_idx], src_ctr) * src_ctr) | ||
| src_geom_dist = np.sqrt( | ||
| np.dot(proj_src_delta, proj_src_delta) * | ||
| proj_src_ctr_dist_sq) |
There was a problem hiding this comment.
src_geom_dist should be dist_geom_src by DM C++ style guide (3.1.0)
There was a problem hiding this comment.
Changed the name to geom_dist_src/ref. geom_dist for "geometric distance".
| @@ -986,9 +1006,14 @@ def _test_spoke(self, src_ctr, src_delta, src_dist, src_ctr_delta, | |||
|
|
|||
| # Compute the cos between our "center" reference vector and the | |||
| # current reference candidate. | |||
| proj_ref_delta = \ | |||
| ref_delta_array[ref_dist_idx] - \ | |||
| np.dot(ref_delta_array[ref_dist_idx], ref_ctr) * ref_ctr | |||
There was a problem hiding this comment.
Implied continuation with parentheses would be more consistent with lines 901-906
| ref_dist_idx_array, ref_delta_array, ref_dist_array, | ||
| def _test_spoke(self, cos_theta_src, sin_theta_src, ref_ctr, ref_ctr_id, | ||
| proj_ref_ctr_delta, proj_ref_ctr_dist_sq, | ||
| ref_dist_idx_array, ref_delta_array, |
There was a problem hiding this comment.
It seems a bit odd to pass in cos and sin theta instead of computing them within the function, as you could mistakenly pass in inconsistent values (i.e. both zero). Why not pass the two vectors instead? If you're doing it to avoid repeated evaluation of cos/sin theta then perhaps the function should allow for iterable inputs.
There was a problem hiding this comment.
I'm not so worried about someone inputting incorrect values for the cosine and sine of an angle here. The method is pretty clearly for use internal to the code and the cosine and sine are computed from the same vectors in the code and will always be consistent with each other. Also, I'm not just using it to avoid repeated calculation but also avoid unnecessary calculation. Inside the loop sine for the ref is only calculated if the cosine comparison succeeds, avoiding an extra set of calculation if the first value does not pass.
| else: | ||
| sin_comparison = \ | ||
| (dot_cross_src - dot_cross_ref) / cos_theta_ref | ||
| (sin_theta_src - sin_theta_ref) / cos_theta_ref | ||
|
|
||
| if abs(sin_comparison) > src_sin_tol: | ||
| continue |
There was a problem hiding this comment.
Does the unit test not require any changes?
Please run it through Jenkins either way. I would suggest testing ci_hsc if it's used in it at all.
Consider filing a tech debt ticket to make the rest of the file more consistent with the style guide.
There was a problem hiding this comment.
All the unittests current pass for each of commits in this ticket and Jenkins will be run before this ticket is merged. The line you comment on here is purely a cosmetic change as before doing the plane project the values of the sines were approximate. Now that they are exact, I've changed the name to explicitly say sine.
As for ci_hsc, the code config uses another matcher in ci_hsc. I'm working on another ticket, DM-14857, to make the pessimisticMatcher the default.
There was a problem hiding this comment.
As for style clean up ticket. I'll talk to @jdswinbank and consider filing one.
Fix variable name typo. Clarify code doc string. reword variable name. Change var names and comment text.
morriscb
force-pushed
the
tickets/DM-15046
branch
from
e9a9574
to
6fd786d
Compare
No description provided.