-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfixes from 870 #1034
Bugfixes from 870 #1034
Conversation
) | ||
# General case where y_vec is not zero. Use arctan2 to determine orientation | ||
length = np.sqrt(x_vec**2 + y_vec**2) | ||
norm_x = (-y_vec / length)[~y_vec.eq(0)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal but instead of computing equality to zero several times, it would probably be more efficient / clear to save it a variable. Also generally floating point number comparison should be computed using np.isclose
Are we assuming that if y_vec
is zero in all the same places x_vec
is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can make those changes
I don't think that assumption is here - I think we're just handling the cases of y_vec
being 0 differently - It there's enough info to determine up/down with X, do so. If all ys are 0 (because of bad data, misaligned leds, etc), raise the error.
# General case where y_vec is not zero. Use arctan2 to determine orientation | ||
length = np.sqrt(x_vec**2 + y_vec**2) | ||
norm_x = (-y_vec / length)[~y_vec.eq(0)] | ||
norm_y = (x_vec / length)[~y_vec.eq(0)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checking that norm_y
should use x_vec
? I didn't look at the calculation too carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this iterrows
version has x_vec
norm in the first arg of arctan
, and -y_vec
in the second arg ...
norm = np.array([-y_vec / length, x_vec / length])
orientation.append(np.arctan2(norm[1], norm[0]))
That became this with version, with the same order of args. By maybe my norm
vars are mislabeled?
norm_x = (-y_vec / length)[~y_vec.eq(0)]
norm_y = (x_vec / length)[~y_vec.eq(0)]
orient[~y_vec.eq(0)] = np.arctan2(norm_y, norm_x)
Description
This fixes two bugs introduced in 870 that had the potential to generate bad data
dlc_utils.py:red_led_bisector
was failing in case where LEDs 1 and 2 were present, but 3 was NaN.dlc_utils.py:Centroid
: one-point centroid had issues with partial NaNs, where NaNs were passed as0
.Using the script below, I verified that no production data was impacted.
Verification Script
Resulting
impact
vars were emptyChecklist:
CITATION.cff
alter
snippet for release notes.CHANGELOG.md
with PR number and description.