fix ICP negative dError. Issue #446 #447
Conversation
* added clause for negative dError * change initiation of errors from constant value to np.inf
|
Thank you for you PR. Your improvement make sense for me. I have some minor requests to improve the whole code. PTAL. |
| indexes, error = nearest_neighbor_association(previous_points, current_points) | ||
| Rt, Tt = svd_motion_estimation(previous_points[:, indexes], current_points) | ||
|
|
||
| if dError < 0: # prevent matrix H changing, exit loop |
There was a problem hiding this comment.
dError is only updated in line 60. If we check the value in line 64 with "dError <= EPS", we can check the value is negative or not. So, I think this check is not needed here. What do you think?
There was a problem hiding this comment.
I put if dError < 0 before the line 58 that changes the state of matrix H. If the error increases, we want to use matrix H from the previous iteration when error was lower. So breaking before line 58 keeps previous iteration transformation.
There was a problem hiding this comment.
we want to use matrix H from the previous iteration
Yes. I know what you want. But, I am not sure your change achieve it.
dError is only updated in line 60, so when dError changed to negative, it always break at line 64 without you change (if dError <= EPS.).
I think this means previous code works based on what you want.
There was a problem hiding this comment.
Yes, you are right. Thank you for watching. My bad, I didn't transfered the code correctly, as I did some more modifications for my project.
- dError should not be absolute, otherwise we cannot catch negative dError.
- dError assignment goes before
if dError < 0check. - Also changed from Converge to Not Converge as when error is increasing it's the second case.
|
I am not sure, do you want the indention in line 41 to be the same level as "(" or "plt" + tab? |
|
I'm sorry I mean this code The second line should be align the top of (`key.... of the first line. |
* correct order for calculating errors * fix indention
|
The indention we were talking about didn't passed the check, so I suggest split function arguments in two lines as I did in last commit. |
| preError = error | ||
|
|
||
| # update current points | ||
| current_points = (Rt @ current_points) + Tt[:, np.newaxis] |
There was a problem hiding this comment.
Could you please move line 61 and 62 to line 50 after svd_motion_estimation? I think this looks more easy to read the code.
| preError = error | ||
| print("Residual:", error) | ||
|
|
||
| if dError <= EPS: |
There was a problem hiding this comment.
We can merge these break statements to line 55 like:
if 0 <= dError <= EPS:
print("Converge", error, dError, count)
break
elif dError < 0:
print("Not Converge... Error increased. ", preError, dError, count)
break
elif MAX_ITER <= count:
print("Not Converge... Reached max iterations", error, dError, count)
break
There was a problem hiding this comment.
I cannot see how we can merge break statements. Because it's important for H = update_homogeneous_matrix to be after dError < 0 (as at this break we take H status from previous iteration) and before dError <= EPS (as we need to update H before possible breaking for these 2 cases).
P.S. I thought it over, it is possible to move up 2 last break statements, but there will always be something between dError < 0 and the others. Moving them up, it is necessary to do dError = preError - error after them but before dError < 0. To my mind it becomes a mess in algorithm logic, so I suggest to keep code as is.
I share your wish to make things beautiful. But here I don't see a way to make it better.
| preError = error | ||
| print("Residual:", error) | ||
|
|
||
| if dError <= EPS: |
Reference issue
fix #446
What does this implement/fix?
Fixes case of negative
dErrorin ICP algorithm.Additional information
Implementation:
When
dErroris negative (error is rising) exits iteration loop with values of previous iterations with lower error.The first iteration is guaranteed by defining
dErrorandpreErroras np.inf before the loop.