DM-18400: Number of matches is smaller than request #119
Conversation
Add clipping minimum for perfect data.
| high=2)[-1], | ||
| 1e-16)) | ||
| n_matched_clipped = np.sum( | ||
| match_sources_struct.distances_rad < clipped_max_dist) |
There was a problem hiding this comment.
There seems to be repeated block of code between lines 324-336 and 354-365. This would be better if it was given some structure, e.g. pulled out into a function. These two blocks also repeat usage of the same variable names, which is both confusing and risky.
| # based on only six points. Here, we refine that result by | ||
| # using all of the good matches from the “final verification” | ||
| # step, above. This will produce a more consistent result. | ||
| fit_shift_rot_matrix = least_squares( |
There was a problem hiding this comment.
This for loop (starting at line 257) is getting really long (> 100 lines) and the fitting stage starting here seems like a different stage compared to the earlier part of the loop; can it be extracted into a separate function? It would be nicer if this part had a name and defined inputs/outputs.
| shift_rot_matrix, | ||
| max_dist_rad, | ||
| min_matches): | ||
| """Match the all sources into the reference cat using the shift/rot |
| matrix. | ||
|
|
||
| After the initial set of matches and found to have enough matches, | ||
| refit the shift/rot matrix for a more stable solution. |
There was a problem hiding this comment.
"and found" probably isn't want you meant, but I'm not sure what you wanted. I don't know how to interpret the imperative phrasing; am I, the user, supposed to do the refitting or is that what this function does? This could use a clear statement of what the output is.
| match_ids=None, | ||
| distances_rad=None, | ||
| max_dist_rad=None, | ||
| ) |
There was a problem hiding this comment.
It would be simpler to return None on failure rather than a struct full of Nones. That way you wouldn't have to generate this at the start, the short-circuit error returns would be more obvious (return None is clearer than return output_struct), and the test in match() would be much simpler.
There was a problem hiding this comment.
Pretty sure others had an opinion against just returning None when I first coded things this way. For consistency with the rest of the code I'm going to leave it as is.
| match_sources_struct.distances_rad < max_dist_rad] | ||
|
|
||
| n_matched = len(cut_ids) | ||
| # Check clipped distances. The minimum value here |
There was a problem hiding this comment.
Still need to extract the clipping calculation into a function rather than repeating it twice.
morriscb
force-pushed
the
tickets/DM-18400
branch
2 times, most recently
from
b5b03af
to
d4cc278
Compare
Debug variable names. Move clipping to its own function. Fix typos. Remove trailing whitespace
morriscb
force-pushed
the
tickets/DM-18400
branch
from
0ae2edd
to
dd815ef
Compare
No description provided.