udacity / sdc-issue-reports

29 stars 6 forks source link

Term 2, Lesson 12, Part 9, Solution Initialize Priors Function #1342

Closed benenaes closed 6 years ago

benenaes commented 6 years ago

The given solution is not correct.

A better solution would be:

std::vector initialize_priors( int map_size, std::vector landmark_positions, float control_stdev) { std::vector priors(map_size, 0); //initialize priors assumimg vehicle at landmark +/- 1.0 meters position stdev

const float landmark_prob = 1.f / (landmark_positions.size() * ((int) control_stdev * 2 + 1));

for (const float landmark_position : landmark_positions)
{
    const unsigned int control_stdev_min_idx =
        max(0, (int) (landmark_position - control_stdev));
    const unsigned int control_stdev_max_idx = 
        min(map_size - 1, (int) (landmark_position + control_stdev));
    for (unsigned int idx = control_stdev_min_idx; idx <= control_stdev_max_idx; ++idx)
    {
        priors[idx] += landmark_prob;
    }
}

return priors;

}

baumanab commented 6 years ago

Thank you for the very detailed feedback and solution suggestion! The current quiz set (coding and concept quizzes) is intended to build familiarity with Bayesian filters through the combination of hand calculations and realization of those calculations in code. As such we did not pursue overlap and boundaries. It may be worth implementing at some point and we will consider that.

With respect to this item:

The function assumes that control_stdev == 1, because only the positions immediately next to a landmark are initialized with 1.0 / normalization_term

The control_stdev is not an assumption but is rather defined 1.0 for purposes of this example. That dictates initialization at a landmark +/- 1.0/normalization term. Does tha make sense?

benenaes commented 6 years ago

OK, but maybe this could be pointed out in the comments. Because if control_stdev would be e.g. 2, then:

    priors[landmark_center] = 1.0f/normalization_term;
    priors[landmark_center - 1] = 1.0f/normalization_term;
    priors[landmark_center + 1] = 1.0f/normalization_term;

would have to become:

    priors[landmark_center] = 1.0f/normalization_term;
    priors[landmark_center - 2] = 1.0f/normalization_term;
    priors[landmark_center + 2] = 1.0f/normalization_term; 
    priors[landmark_center - 1] = 1.0f/normalization_term;
    priors[landmark_center + 1] = 1.0f/normalization_term;
baumanab commented 6 years ago

That is a good point. A better approach for the solution would probably be initializing priors based on the control standard deviation.

baumanab commented 6 years ago

A note has been added to the quiz solution.

Nk911 commented 6 years ago

Please check my query on the same topic here , as it still have errors https://carnd.slack.com/files/U8Q0TD085/FA1E1CQR2/Term2_-_Markov_Localization___control_stdev_vs_position_stdev

https://discussions.udacity.com/t/control-standard-deviation-vs-position-standard-deviation/655523

Fix it if I am correct.