Smooth Reward Function / Safe Driving

[bhairavmehta95]

Implements a simple safe driving penalty for use for collision detection. (Implemented as described in #24).

We will almost definitely need to scale the rewards from this and the rest of the environment to get the behavior that we are looking for.

[bhairavmehta95]

Will fix the failing test tomorrow.

[maximecb]

Just two nitpicks:

• Can you use a more descriptive name for DUCKIE_SR and other constants. Also note that I've been referring to the duckiebot elsewhere in the code as robot or agent
• The statement after if or else should go on a separate line.

[bhairavmehta95]

Fixed.

Also, it may be beneficial to merge #32 first, and then this one if that's okay?

Just because there were fixes to things like draw-bbox on that PR. In the future, I will try to branch out fixes like that (i.e ones that break certain functionality) and merge them into master as soon as possible. Sorry for the inconvenience.

[maximecb]

Apologies, I seem to have created more conflicts.

[maximecb]

I had removed the pos argument from _actual_center(), and made the agent corners local variables in the collision check function.

[bhairavmehta95]

Should be fixed now, as long as the tests pass.

Only thing is that we need to keep the agent corners member variables to ensure that the draw-bbox functionality works correctly (which is what I had to change in the last commit).

[maximecb]

I tested the PR and took a closer look. Some feedback.

1. I don't think this is accurate:

safety_radius = SAFETY_RAD_MULT * np.amax(
    [abs(mesh.min_coords), abs(mesh.max_coords)]) * scale

This isn't quite right if we want a circle that wraps the entire bounding box, which is then multiplied by a scaling factor. You need to first compute the max absolute x and z, then compute sqrt(x^2 * + z^2).

2. You re-added the pos argument to _actual_center() :(

3. object_corners is missing a comment

4. I think _safe_driving() needs a more descriptive name, maybe just _safety_penalty()

I think this is it, it seems fine otherwise.

[bhairavmehta95]

Oh my bad! I got confused when cleaning the merge conflicts so I must have switched the wrong code.

Will fix asap

On Tue, Jul 10, 2018, 2:25 PM Maxime Chevalier-Boisvert < notifications@github.com> wrote:

I tested the PR and took a closer look. Some feedback.

1. I don't think this is accurate:

safety_radius = SAFETY_RAD_MULT np.amax( [abs(mesh.min_coords), abs(mesh.max_coords)]) scale

This isn't quite right if we want a circle that wraps the entire bounding box, which is then multiplied by a scaling factor. You need to first compute the max absolute x and z, then compute sqrt(x^2 * + z^2).

1.

You re-added the pos argument to _actual_center() :( 2.

object_corners is missing a comment 3.

I think _safe_driving() needs a more descriptive name, maybe just _safety_penalty()

I think this is it, it seems fine otherwise.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/duckietown/gym-duckietown/pull/36#issuecomment-403920720, or mute the thread https://github.com/notifications/unsubscribe-auth/AKBGMcMZd19QuySEI6ficYoRtOgUUfaGks5uFPGMgaJpZM4VIZVK .