Skip to content
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

clustering integration #45

Closed
wants to merge 7 commits into from
Closed

clustering integration #45

wants to merge 7 commits into from

Conversation

ashum68
Copy link
Contributor

@ashum68 ashum68 commented Aug 21, 2024

No description provided.

@ashum68
Copy link
Contributor Author

ashum68 commented Aug 21, 2024

still writing tests, just wanted to put this here earlier so there's more time to review.

@ashum68 ashum68 force-pushed the clustering-integration branch from bf9a02c to 26c7fae Compare August 21, 2024 21:47
Copy link
Contributor

@TongguangZhang TongguangZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed

Comment on lines 24 to 25
OBSTACLES = 0
DETECTIONS = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this meant to be an enum? if so use an enum class, and they shouldn't both be 0 right?

detection_input_queue: queue_wrapper.QueueWrapper,
obstacle_input_queue: queue_wrapper.QueueWrapper,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need 2 queues if we'll only run either obstacles or detections but not both at the same time?

Comment on lines 63 to 68
try:
odometry: drone_odometry_local.DroneOdometryLocal = (
odometry_input_queue.queue.get_nowait()
)
except queue.Empty:
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this must happen not matter the merge data type, you should move it outside after the if statement

Comment on lines 59 to 60
if len(obstacles) == 0:
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do we skip the delay if the length is 0? why is this a special case?

Comment on lines 82 to 84
if len(detections) == 0:
continue
time.sleep(delay)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

detections = []
obstacles = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need 2 variables if we'll only run either obstacles or detections but not both at the same time?

Comment on lines 96 to 101
if str(type(merged_data)) == "<class 'detections_and_odometry.DetectionsAndOdometry'>":
return self.run_simple_decision(
self.merged_odometries, self.proximity_limit, current_flight_mode
)
if str(type(merged_data)) == "<class 'obstacles_and_odometry.ObstaclesAndOdometry'>":
return self.run_obstacle_avoidance(self.merged_odometries)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use the string of the classes, pass in the method instead - maybe instead of passing a string from main to data merge, pass a data merge enum, and here, use the enum as well

Comment on lines 40 to 51
obstacle_data: obstacle.Obstacle = obstacle_in_queue.queue.get_nowait()
if obstacle_data is None:
break

result, value = decider.run(merged_data)
if not result:
continue

result, value = decider.run_obstacle_avoidance(obstacle_data)
if not result:
continue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the config passed in from main to decide what to run - we're not running both

Comment on lines 50 to 51
detection_to_clustering_queue.queue.put(value)
detection_to_data_merge_queue.queue.put(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't do both

Copy link
Contributor

@TongguangZhang TongguangZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed

Comment on lines +203 to +204
merged_to_clustering_queue.fill_and_drain_queue()
clustering_to_deflection_queue.fill_and_drain_queue()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check not None

@ashum68 ashum68 closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants