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

[AVR-761] Lberg/refactor map api #196

Merged
merged 21 commits into from
Jan 13, 2021
Merged

Conversation

lucabergamini
Copy link
Contributor

@lucabergamini lucabergamini commented Nov 10, 2020

Refactor of MapAPI:

  • move bounds computation inside the mapAPI. This makes the mapAPI object more independent;
  • perform interpolation of lanes. This allows to speed up transformations between spaces

anasrferreira
anasrferreira previously approved these changes Nov 10, 2020
Copy link

@anasrferreira anasrferreira left a comment

Choose a reason for hiding this comment

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

I am guessing that the goal of exposing this is that we can select the elements of interest by their bounds, right?
Though for the geometry itself we will want to use get_lane_coords and get_crosswalk_coords
Is this right @lucabergamini

@lucabergamini
Copy link
Contributor Author

I am guessing that the goal of exposing this is that we can select the elements of interest by their bounds, right?
Though for the geometry itself we will want to use get_lane_coords and get_crosswalk_coords

Exactly :)

@lucabergamini
Copy link
Contributor Author

PTAL @anasrferreira , I've added a function to sample from the lane coords

anasrferreira
anasrferreira previously approved these changes Nov 11, 2020
Copy link

@anasrferreira anasrferreira left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -15,7 +15,7 @@
CV2_SHIFT_VALUE = 2 ** CV2_SHIFT


def elements_within_bounds(center: np.ndarray, bounds: np.ndarray, half_extent: float) -> np.ndarray:
def indices_in_bounds(center: np.ndarray, bounds: np.ndarray, half_extent: float) -> np.ndarray:

Choose a reason for hiding this comment

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

Should this one also get moved to map_api?

l5kit/l5kit/data/map_api.py Outdated Show resolved Hide resolved
anasrferreira
anasrferreira previously approved these changes Nov 12, 2020
l5kit/l5kit/data/map_api.py Show resolved Hide resolved
anasrferreira
anasrferreira previously approved these changes Nov 18, 2020
@lucabergamini lucabergamini changed the title Lberg/refactor map api [AVR-761] Lberg/refactor map api Dec 4, 2020
* improve transform through interpolation

* add comment

* rename variable
perone
perone previously approved these changes Jan 12, 2021
Copy link
Contributor

@perone perone left a comment

Choose a reason for hiding this comment

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

Just minor comments 👍

l5kit/l5kit/data/map_api.py Outdated Show resolved Hide resolved
Comment on lines +16 to +17
INTER_METER = 0 # fixed interpolation at a given step in meters
INTER_ENSURE_LEN = 1 # ensure we always get the same number of elements
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'll need to add #: to autodoc be able to render this doc.

l5kit/l5kit/data/map_api.py Outdated Show resolved Hide resolved
l5kit/l5kit/data/map_api.py Outdated Show resolved Hide resolved
l5kit/l5kit/data/map_api.py Outdated Show resolved Hide resolved
Comment on lines +174 to +176
xyz_inter[:, 0] = np.interp(steps, xp=cum_dist, fp=xyz[:, 0])
xyz_inter[:, 1] = np.interp(steps, xp=cum_dist, fp=xyz[:, 1])
xyz_inter[:, 2] = np.interp(steps, xp=cum_dist, fp=xyz[:, 2])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about it but I think that the np.interp would allow you to do this on a single call, but have to double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to the docstring it's only 1D and it doesn't take axis as an argument :(

l5kit/l5kit/data/map_api.py Show resolved Hide resolved
l5kit/l5kit/rasterization/semantic_rasterizer.py Outdated Show resolved Hide resolved
l5kit/l5kit/rasterization/semantic_rasterizer.py Outdated Show resolved Hide resolved
l5kit/l5kit/data/map_api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@yawei-ye yawei-ye left a comment

Choose a reason for hiding this comment

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

Leave some suggestions on function names, otherwise LGTM

l5kit/l5kit/data/map_api.py Outdated Show resolved Hide resolved
l5kit/l5kit/data/map_api.py Show resolved Hide resolved
l5kit/l5kit/data/map_api.py Show resolved Hide resolved
l5kit/l5kit/data/map_api.py Show resolved Hide resolved
return False
traffic_el = element.element.traffic_control_element
return traffic_el.HasField("traffic_light") is True

def is_traffic_face_colour(self, element_id: str, colour: str) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def is_traffic_face_colour(self, element_id: str, colour: str) -> bool:
def check_traffic_light_state(self, element_id: str, tl_state: str) -> bool:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

state is something like `ACTIVE, INACTIVE, here we're just checking if the element is a traffic light face and which colour it has. It's a static info, not a dynamic one

lanes_lines[lane_type].extend([xy_left, xy_right])
for lane_area in lanes_area.reshape((-1, INTERPOLATION_POINTS * 2, 2)):
# need to for-loop otherwise some of them are empty
cv2.fillPoly(img, [lane_area], (17, 17, 31), **CV2_SUB_VALUES)
Copy link
Contributor

Choose a reason for hiding this comment

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

where does (17, 17, 31) come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no idea, it was there before I started working on this

Copy link
Contributor

@yawei-ye yawei-ye left a comment

Choose a reason for hiding this comment

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

Minor

l5kit/l5kit/data/map_api.py Outdated Show resolved Hide resolved
yawei-ye
yawei-ye previously approved these changes Jan 13, 2021
Copy link
Contributor

@yawei-ye yawei-ye left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@perone perone left a comment

Choose a reason for hiding this comment

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

Looks good to me ! 👍

@lucabergamini lucabergamini merged commit ede8c04 into master Jan 13, 2021
@lucabergamini lucabergamini deleted the lberg/refactor-map-api branch January 13, 2021 12:35
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.

5 participants