# Write _construct_pattern_and_shift_rot_matrix in C++/pybind

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
10
• Sprint:
AP F21-6 (November), AP S22-1 (December)
• Team:
• Urgent?:
No

#### Description

Write C++ code wrapped with pybind11 to replace the _construct_pattern_and_shift_rot_matrix, associated loops, and sub-methods.

#### Activity

Hide
Chris Morrison [X] (Inactive) added a comment - - edited

Ran through the RC2 dataset as used in previous tickets. No additional detectors were lost due to slight changes in the algorithm. Below are the timings in seconds.

This ticket:
Mean: 1.271 STD: 0.499
Percentiles: 5%,  25%,  50%,  75%,     95%
0.870, 1.039, 1.194, 1.407, 1.884
Min: 0.635, Max: 28.494

Compared to the previous timings in DM-32010, this represents a ~60% reduction in the mean time and a ~10 fold decrease in scatter.

The code as committed and tested represents a slight change in algorithm. I've moved the python intermediate_verify step outside of the the main loop over reference candidates (now in the C++ layer). This was done as coding the least squares fit in C++ would take to long compared to the time I have left on my employment. There are no additional failures caused by this change.

There are likely specific speed ups still available in the code, specifically in the create_sorted_arrays method. That method uses std::vectors and push back to create it's arrays which is inefficient for creating the sorted distance array and it's argsort.

Show
Chris Morrison [X] (Inactive) added a comment - - edited Ran through the RC2 dataset as used in previous tickets. No additional detectors were lost due to slight changes in the algorithm. Below are the timings in seconds. This ticket: Mean: 1.271 STD: 0.499 Percentiles: 5%,  25%,  50%,  75%,     95%                  0.870, 1.039, 1.194, 1.407, 1.884 Min: 0.635, Max: 28.494 Compared to the previous timings in DM-32010 , this represents a ~60% reduction in the mean time and a ~10 fold decrease in scatter. Jenkins: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/35590/pipeline The code as committed and tested represents a slight change in algorithm. I've moved the python intermediate_verify step outside of the the main loop over reference candidates (now in the C++ layer). This was done as coding the least squares fit in C++ would take to long compared to the time I have left on my employment. There are no additional failures caused by this change. There are likely specific speed ups still available in the code, specifically in the create_sorted_arrays method. That method uses std::vectors and push back to create it's arrays which is inefficient for creating the sorted distance array and it's argsort.
Hide
Chris Morrison [X] (Inactive) added a comment -

Sending it to you Ian as I won't be able to review the full ticket. Jenkins is failing but in pipe_base which looks like possibly a break on master. meas_astrom passes.

Show
Chris Morrison [X] (Inactive) added a comment - Sending it to you Ian as I won't be able to review the full ticket. Jenkins is failing but in pipe_base which looks like possibly a break on master. meas_astrom passes.
Hide
Chris Morrison [X] (Inactive) added a comment -

Added link to ticket DM-32985 which regards implementing the intermediate_verify fit and fully match the Python code. However, this is optional as I do not believe the fitting step here in the loop as any significant effect on the algorithm as implemented.

Show
Chris Morrison [X] (Inactive) added a comment - Added link to ticket DM-32985 which regards implementing the intermediate_verify fit and fully match the Python code. However, this is optional as I do not believe the fitting step here in the loop as any significant effect on the algorithm as implemented.
Hide
Krzysztof Findeisen added a comment -

Code looks good; I just realized Ian Sullivan probably didn't get the GitHub notification.

Show
Krzysztof Findeisen added a comment - Code looks good; I just realized Ian Sullivan probably didn't get the GitHub notification.
Hide
Ian Sullivan added a comment -

I didn't, thanks for pointing it out to me.

Show
Ian Sullivan added a comment - I didn't, thanks for pointing it out to me.
Hide
John Parejko added a comment -
Show
John Parejko added a comment - Jenkins: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/35839/pipeline

#### People

Assignee:
John Parejko
Reporter:
Chris Morrison [X] (Inactive)
Reviewers:
Krzysztof Findeisen
Watchers:
Chris Morrison [X] (Inactive), Eric Bellm, Ian Sullivan, John Parejko, Krzysztof Findeisen