udacity / sdc-issue-reports

29 stars 6 forks source link

Term 3, lesson 4, 13. Hybrid A* Pseudocode - Confusing Pseudocode #1447

Closed DominikMa closed 5 years ago

DominikMa commented 5 years ago

In the given pseudocode there are some confusing or even wrong code lines.

  1. On line 49 and 53 the function index is used while on line 65 and 78 the function idx is used for the same purpose - maybe you should decide for one
  2. On line 29 and 32 [[[0 for x in range(grid[0])] for y in range(len(grid))] for cell in range(NUM_THETA_CELLS)] maybe should be [[[0 for x in range(len(grid[0]))] for y in range(len(grid))] for cell in range(NUM_THETA_CELLS)] unless you stored the x size of the grid actually in grid[0]
  3. On line 65 if (idx(current.x) == goal[0]) and (idx(current.y) == goal.y): maybe should be if (idx(current.x) == goal.x) and (idx(current.y) == goal.y):
  4. On line 73 if next_states is not in the grid: maybe should be if next_state is not in the grid:
  5. While not necessarily wrong it would be a bit nicer to even use the variable you are returning. The path variable, returned on line 67, it never accessed.

Additionally I would suggest not filling the array came_from with 0s especially if you later fill it with State objects. Since it is pseudocode it should be easily readable, so why not using something like None? Same goes for closed - if it is python like pseudocode why not using True and False?

The state.g value is not explained anywhere. Surely it is a counter for how many expansions there where before each state, but what for? Tracking the length? Calculating the path? If it appears in the pseudocode it should be clear for what or it should be explained before.

The way NUM_THETA_CELLS is used is not clearly explained anywhere. If I get it right from the code, we now can visit each grid cell multiple times with different thetas. As this is not the same as in the previous explanations (where we can visit each cell exactly one) it might be usefully to explain the concept.

Finally I think there is a typo in the description.

maybe should be

mvirgo commented 5 years ago

Hi there! With Waffle closing its doors next month, and the ability to leave more detailed feedback on each classroom page (see “Send Feedback” in the upper right if you are enrolled in the program), we are migrating away from our Waffle board. All issues and conversations here will be migrated onto our internal platforms.

Over the next week, I am leaving these notes in case anyone has further follow-ups they want to get in before the board closes, as well as migrating over the tickets. As of 4/19, I will close all the remaining open tickets, and archive the related Github repository.