DM-32010: Write `_create_pattern_spokes` in C++/pybind #156

morriscb · 2021-11-22T18:58:24Z

No description provided.

kfindeisen

Looks good; mostly style/clarity comments, though I do have a few algorithmic questions about the indexing.

kfindeisen · 2021-11-22T19:23:00Z

include/lsst/meas/astrom/pessimisticPatternMatcherUtils.h

+/**
+ * @param[in] src_dist float value of the distance we would like to search for
+ *     in the reference array in radians.
+ * @param[in] ref_dist_array sorted array of distances in radians.
+ * @param[in] max_dist_rad maximum plus/minus search to find in the reference
+ *     array in radians.
+ */
+std::vector<size_t> find_candidate_reference_pair_range(float src_dist,


Please provide a full documentation block, following DM guidelines. The information required is more or less the same as for Python Numpydoc, even if the presentation is different.

I'm especially curious about what the return value is, if it needs to be a vector...

Changed to pair and updated doc.

kfindeisen · 2021-11-22T19:55:55Z

include/lsst/meas/astrom/pessimisticPatternMatcherUtils.h

+ * Create the individual spokes that make up the pattern now that the
+ * shift and rotation are within tolerance.
+ *
+ * If we can't create a valid pattern we exit early.


This statement is unclear. Do you return a partial result, or throw an exception? If the latter, which one?

kfindeisen · 2021-11-22T19:56:05Z

include/lsst/meas/astrom/pessimisticPatternMatcherUtils.h

+ * @param[in] max_dist_rad Maximum search radius for distances.
+ * @param[in] n_match Number of source deltas that must be matched into the
+ *     reference deltas in order to consider this a successful pattern match.
+ */


Please document the return value.

kfindeisen · 2021-11-22T20:05:12Z

include/lsst/meas/astrom/pessimisticPatternMatcherUtils.h

- *     array. uint16 type is to limit the amount of memory used and is set
- *     in the pessimistic_pattern_matcher_3d python class with reference
- *     catalogs trimmed by the matchPessimisticB runner class.
+ *     array.
 * @param[in] proj_ref_ctr_delta Plane projected first spoke in the reference
 *     pattern using the pattern center as normal.
 * @param[in] proj_ref_ctr_dist_sq Squared length of the projected vector.
 * @param[in] ref_dist_idx_array Indices sorted by the delta distance between


This parameter was renamed to candidate_range.

include/lsst/meas/astrom/pessimisticPatternMatcherUtils.h

kfindeisen · 2021-11-23T00:07:41Z

src/pessimisticPatternMatcherUtils.cc

+        std::vector<size_t> tmpIds{ref_id, src_idx + 1};
+        output_spokes.push_back(tmpIds);
+        if (output_spokes.size() >= n_match - 2) {
+            return output_spokes;


I'd suggest using break here, or return output_spokes up above, to make this function's logic easier to follow. Right now it's not clear that these two cases (enough successes or too many failures) are handled the same way.

kfindeisen · 2021-11-23T00:14:07Z

src/pessimisticPatternMatcherUtils.cc

    // Loop over our candidate reference objects. ref_dist_idx_array is a view into ref_id_array
    // and is not the same length as ref_id_array.


Out-of-date reference to ref_dist_idx_array.

kfindeisen · 2021-11-23T00:15:05Z

src/pessimisticPatternMatcherUtils.cc

-        unsigned int ref_id = ref_id_array[*idx];
+        if (idx % 2 == 0) {
+            midpoint = midpoint + idx;
+            ;


Empty statement.

kfindeisen · 2021-11-23T00:18:02Z

src/pessimisticPatternMatcherUtils.cc

        // Compute the delta vector from the pattern center.
-        unsigned int ref_id = ref_id_array[*idx];
+        if (idx % 2 == 0) {
+            midpoint = midpoint + idx;
+            ;
+        } else {
+            midpoint = midpoint - idx;
+        }


I'd recommend factoring this logic into its own function, like you did in Python; it's much harder to figure out what these alternating indices do when they're embedded in a larger data access loop.

Hmm... I'm a little hesitant doing this as it would mean modifying midpoint in place in the external function. I've change things around slightly now that I'm just returning the start and end value from find_candidate.

I guess I assumed that the function could return a vector giving the correct access order, and midpoint would become an iteration variable over that vector instead of idx (which you seem to use exclusively for calculating midpoint).

I suppose. I was thinking once all the functions that use find_candidate are in C++ i could write something where the find_candidate method returns and iterator that does what you describe. I'm hesitant to create a vector as I don't want to have to loop through this data more than once. Depending on the tolerance, the iterations these steps do can be pretty large.

Fair enough -- I realize this is still work in progress.

That said, creating an iterator is a lot more work in C++ than in Python (no generators or yield), so I was suggesting the vector of indices as a quick solution.

Yeah, I figured. I think I'm happy enough with it for now but could add some extra comments.

kfindeisen · 2021-11-23T00:21:31Z

src/pessimisticPatternMatcherUtils.cc

+
+        /// Append the successful indices to our list. The src_idx needs
+        /// an extra iteration to skip the first and second source objects.
+        std::vector<size_t> tmpIds{(size_t)ref_id, src_idx + 1};


Please use modern casts: static_cast<size_t>(ref_id).

Debug findCandidateReferencePairs Finish up findCandidateReferencePairs Create C++ create_pattern_spokes method. Simplify find_candidate_reference_pair_range Fix compliation for find_candidate. Debug find_candiate_pairs. Change code to use c++ candidate pairs. Debug indexing. Finish up porting create_pattern_spokes method. Debug typting. Hook up new c++ code. Fixup typing. Hook up using create_pattern_spokes Debug python side. Disconnect python method. Fix linting. Fix linting. Move around astype Revert Add docs. Remove python test_spoke method. Clang formating. Reverse generator order. Cast int to size_t Respond to reviewer. Fix header type. Change internal type. Fix accessor. Fix candidate range accessing. Debug generator. Remove unused import. Update docs. Remove unnamed namespace. Explicitly declare pybind args. Add pybind literals. Remove ref_id from doc. Store eigen conversions. Finish storing eigen variables. Fix typo

morriscb force-pushed the tickets/DM-32010 branch from 3ca1c78 to 17c6f0e Compare November 22, 2021 19:00

kfindeisen approved these changes Nov 23, 2021

View reviewed changes

morriscb force-pushed the tickets/DM-32010 branch from 4fa5847 to 592fb80 Compare November 24, 2021 00:53

morriscb force-pushed the tickets/DM-32010 branch from 592fb80 to 041cc4a Compare November 24, 2021 02:05

morriscb merged commit 44dc1dc into master Nov 24, 2021

morriscb deleted the tickets/DM-32010 branch November 24, 2021 04:34

kfindeisen mentioned this pull request Jan 5, 2022

DM-32299: Write _construct_pattern_and_shift_rot_matrix in C++/pybind #158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-32010: Write `_create_pattern_spokes` in C++/pybind #156

DM-32010: Write `_create_pattern_spokes` in C++/pybind #156

morriscb commented Nov 22, 2021

kfindeisen left a comment

kfindeisen Nov 22, 2021

morriscb Nov 23, 2021

kfindeisen Nov 22, 2021

kfindeisen Nov 22, 2021

kfindeisen Nov 22, 2021

kfindeisen Nov 23, 2021

kfindeisen Nov 23, 2021

kfindeisen Nov 23, 2021

kfindeisen Nov 23, 2021

morriscb Nov 23, 2021

kfindeisen Nov 23, 2021 •

edited

morriscb Nov 23, 2021

kfindeisen Nov 23, 2021 •

edited

morriscb Nov 23, 2021

kfindeisen Nov 23, 2021

		// Loop over our candidate reference objects. ref_dist_idx_array is a view into ref_id_array
		// and is not the same length as ref_id_array.

DM-32010: Write _create_pattern_spokes in C++/pybind #156

DM-32010: Write _create_pattern_spokes in C++/pybind #156

Conversation

morriscb commented Nov 22, 2021

kfindeisen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Nov 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Nov 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DM-32010: Write `_create_pattern_spokes` in C++/pybind #156

DM-32010: Write `_create_pattern_spokes` in C++/pybind #156

kfindeisen Nov 23, 2021 •

edited

kfindeisen Nov 23, 2021 •

edited