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

[Redesign] Add type annotations to operator and component creator #278

Open
wants to merge 3 commits into
base: redesign
Choose a base branch
from

Conversation

HivaMohammadzadeh1
Copy link

No description provided.

@pschafhalter pschafhalter changed the title Adding type annotations to operator_creater.py and component_creater.py [Redesign] Add type annotations to operator and component creator Aug 2, 2022
Copy link
Member

@pschafhalter pschafhalter left a comment

Choose a reason for hiding this comment

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

Left some feedback. It might be a good idea to break this PR into smaller pieces.

Comment on lines +443 to +451
def add_prediction(obstacles_tracking_stream: Stream[ObstaclesMessageTuple],
vehicle_id_stream: Stream[int],
release_sensor_stream: Stream[None],
point_cloud_stream: Stream[PointCloud],
lidar_setup: Stream[LidarSetup],
pose_stream: Optional[Stream[pylot.utils.Pose]] = None,
Camera_transform = None,
time_to_decision_stream: Optional[Stream[float]] = None
) -> Tuple[Stream[prediction], Stream[int], Stream]:
Copy link
Member

Choose a reason for hiding this comment

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

Could you make sure that the arguments remain in the same order as before?

Copy link
Member

Choose a reason for hiding this comment

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

Also, the return type should be Tuple[Stream[List[ObstaclePrediction]], Stream[SegmentedFrame], Stream[None]]

@@ -226,7 +235,7 @@ def add_traffic_light_detector(


def add_traffic_light_invasion_sensor(ground_vehicle_id_stream: Stream,
pose_stream: Stream) -> Stream:
pose_stream: Stream[pylot.utils.Pose]) -> Stream:
Copy link
Member

Choose a reason for hiding this comment

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

Please add an annotation the returned stream with the message type.

camera_setup,
name='center_track'):
def add_center_track_tracking(bgr_camera_stream: Stream[CameraFrame],
camera_setup: Stream[CameraSetup],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
camera_setup: Stream[CameraSetup],
camera_setup: CameraSetup,

global_trajectory_stream,
prediction_stream: Stream[prediction],
time_to_decision_stream: Optional[Stream[float]] = None,
pose_stream: Optional[Stream[pylot.utils.Pose]] = None) -> Stream:
Copy link
Member

Choose a reason for hiding this comment

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

Please annotate the return type of the stream -- it should carry waypoints.

@@ -6,12 +6,14 @@
from absl import flags

from erdos import Stream
from pylot import prediction
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from pylot import prediction

@@ -7,9 +7,13 @@

import erdos
from erdos import Stream
from pylot import prediction
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from pylot import prediction

Comment on lines +30 to +34

from pylot.loggers.pose_logger_operator import PoseLoggerOperator
from pylot.loggers.imu_logger_operator import IMULoggerOperator
from pylot.prediction import obstacle_prediction

Copy link
Member

Choose a reason for hiding this comment

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

Why were these imports added?

@@ -399,7 +408,7 @@ def add_segmentation_decay(


def add_linear_prediction(tracking_stream: Stream,
time_to_decision_stream: Stream) -> Stream:
time_to_decision_stream: Stream) -> Stream[prediction]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
time_to_decision_stream: Stream) -> Stream[prediction]:
time_to_decision_stream: Stream[float]) -> Stream[List[ObstaclePrediction]]:

def add_r2p2_prediction(point_cloud_stream, obstacles_tracking_stream,
time_to_decision_stream, lidar_setup):
def add_r2p2_prediction(point_cloud_stream: Stream[PointCloud], obstacles_tracking_stream: Stream[ObstaclesMessageTuple],
time_to_decision_stream, lidar_setup: Stream[LidarSetup]) -> Stream[prediction]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
time_to_decision_stream, lidar_setup: Stream[LidarSetup]) -> Stream[prediction]:
time_to_decision_stream: Stream[float], lidar_setup: LidarSetup) -> Stream[List[ObstaclePrediction]]:

Comment on lines 442 to 443
tracking_stream: Stream,
prediction_stream: Stream,
Copy link
Member

Choose a reason for hiding this comment

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

Please annotate the types of these streams.

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