-
Notifications
You must be signed in to change notification settings - Fork 0
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
integrate payload order algorithm and wrote up unit test (Not Ready to Merge) #170
base: main
Are you sure you want to change the base?
Conversation
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.
Besides the line-by-line comments, can you also reorganize your code on a high-level a little to make it simpler to read?
Instead of returning a single payload_results
, make test_with_dataset
return two arrays: scores
and distances
, both sorted in payload ranking order. Then average them after (btw you can use np.mean
instead of summing and dividing).
And the way you did the sorting within test_with_dataset
and built the payload results is so convoluted I can't follow it without locking in for a solid 10 minutes at least. I'll try giving some suggestions to simplify it once you've addressed the other comments
skiprows = 1, usecols= range(1,10)) | ||
letter_matrix = np.genfromtxt(os.path.join( irl_root_folder, f"letter_confusion_matrix.csv"), delimiter=',', dtype=str) | ||
letter_matrix = letter_matrix[1:, 1:].astype(np.float32) | ||
#print(letter_matrix) |
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.
can you get rid of all the lines that are commented out like this?
irl_root_folder = f"{CURRENT_FILE_PATH}/visualizations/test_irl" | ||
|
||
|
||
shape_matrix = np.loadtxt( os.path.join( irl_root_folder, f"shape_confusion_matrix.csv"), delimiter=",", |
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.
Can you encapsulate the payload sorting algorithm more? I would rather that when we use it, we don't have to deal with loading all the confusion matrices ourselves.
skiprows= 1, usecols= range(1,9)) | ||
|
||
#print("content", dir(all_ground_truth[0])) | ||
payload_truth = [] |
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.
nitpick on variable names: can you call this ground_truth_descriptors
?
Also, it would be easier to read if you initialize it in one line as a list comprehension.
Summary
For each run of the 3d_dataset, I insert the simple payload order algorithm in the integration unit test. To verify how reliable the payload order algorithm is, I relied on checking if the scores, a list of the number of times the description detected was close enough to the truth, was decreasing and if the list of the 5 target's track distance is increasing.
Running the payload order algorithm requires the letter and shape confusion matrices. I used the irl unit test generated confusion matrices for running the algorithm. When not using penalty, the order of the payload didn't appear to change. When using penalty, the order changes better, but the worst target was not last.
Test Plan
If the placement of the worst target detected was ignored, the track distance of the successfully tracked object were ordered in an increasing order. There might imply that it is the confusion matrix influencing the order of the payload.
Issues
Issue is that the unit test doesn't know how to handle a person test case. With letter and color, there is no specific probability value to assign when shape is a person. I will likely work on that after making this PR update.