[Proposal] Reduce internal dependancy #303

Chachay · 2020-03-20T11:19:08Z

Utilize scipy module to reduce internal dependency (cubic spline)
Reduce high context parameters in rear_wheel_feedback_control function

Other proposal (not implemented):

Speed profile is disabled; could direction switching be decided at line 143? this would be closer to real-world implementation.
pi_2_pi could also be replaced to numpy unwrap; though I think it is not so effective on readability.

lgtm-com · 2020-03-20T11:26:26Z

This pull request introduces 2 alerts when merging 7473907 into 4093b64 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Unused import

AtsushiSakai · 2020-03-23T11:43:06Z

@Chachay Thank you for your PR!!. It looks simpler. Please fix some minor points to merge as below.

AtsushiSakai · 2020-03-23T11:44:15Z

Please fix the issues reported by lgtm-com.

AtsushiSakai

PTAL.

AtsushiSakai · 2020-03-23T11:44:55Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

+
+        self.length = s[-1]
+
+    def yaw(self, s):


calc_yaw is better? function should be start by a verb.

AtsushiSakai · 2020-03-23T11:45:45Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

+        dx, dy = self.dX(s), self.dY(s)
+        return np.arctan2(dy, dx)
+
+    def curvature(self, s):


calc_curvature?

AtsushiSakai · 2020-03-23T11:46:15Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

+        ddx, ddy   = self.ddX(s), self.ddY(s)
+        return (ddy * dx - ddx * dy) / ((dx ** 2 + dy ** 2)**(3 / 2))
+
+    def __findClosestPoint(self, s0, x, y):


Please use snake-case based on pep8.

AtsushiSakai · 2020-03-23T11:48:03Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

+        return (ddy * dx - ddx * dy) / ((dx ** 2 + dy ** 2)**(3 / 2))
+
+    def __findClosestPoint(self, s0, x, y):
+        def f(_s, *args):


Can you make the function name more explicit?

AtsushiSakai · 2020-03-23T11:48:17Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

+            _x, _y= self.X(_s), self.Y(_s)
+            return (_x - args[0])**2 + (_y - args[1])**2
+
+        def jac(_s, *args):


Can you make the function name more explicit?

named calc distance & its jacobian

AtsushiSakai · 2020-03-23T11:48:32Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

+        minimum = optimize.fmin_cg(f, s0, jac, args=(x, y), full_output=True, disp=False)
+        return minimum
+
+    def TrackError(self, x, y, s0):


Should be calc_track_error

AtsushiSakai · 2020-03-23T11:49:49Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

+
+        return e, k, yaw, s

 def PIDControl(target, current):


Please change pid_control

AtsushiSakai · 2020-03-23T11:51:36Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

+        self.yaw = self.yaw + self.v / L * math.tan(delta) * dt
+        self.v   = self.v + a * dt
+
+class TrackSpline:


CubicSplinePath is better?

- rename functions - add target speed controller (as the substitute of calc speed profile)

- remove unnecessary else case

Chachay · 2020-03-24T10:28:52Z

Thank you for your review. I added "switching direction" in addition to the function name fixes.

AtsushiSakai

PTAL

AtsushiSakai · 2020-03-24T12:11:45Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

-    target_speed = 10.0 / 3.6
-
-    sp = calc_speed_profile(cx, cy, cyaw, target_speed)
+    track = CubicSplinePath(ax, ay)


track -> cubic_spline_path ?

couldn't this object be better to have the name detached from the interpolation method?

Does this object mean a spline path object? What do you imply the "track" name?

I meant the name could be any of reference_track, reference_path, target_track and target_path or even only path or track, but it isn't necessary to be cubic spline as long as class C2.

oh ok. I got it. i feel reference_path is better.

AtsushiSakai

PTAL

AtsushiSakai · 2020-03-24T12:18:05Z

PathTracking/rear_wheel_feedback/rear_wheel_feedback.py

-    ind, e = calc_nearest_index(state, cx, cy, cyaw)
-
-    k = ck[ind]
+def rear_wheel_feedback_control(state, e, k, yaw_r):


yaw_r -> yaw_ref ?

I will rename this

AtsushiSakai

LGTM. Thank you!!

Chachay added 2 commits March 20, 2020 10:13

Integrate update function into state class

19630bc

Replace Spline module to Scipy interpolate

7473907

AtsushiSakai self-requested a review March 23, 2020 11:40

AtsushiSakai requested changes Mar 23, 2020

View reviewed changes

Chachay added 2 commits March 23, 2020 16:00

- remove unsued import and variable

a7c82cc

- rename functions - add target speed controller (as the substitute of calc speed profile)

- Changed Class name (CubicSplinePath)

7005a66

- remove unnecessary else case

AtsushiSakai requested changes Mar 24, 2020

View reviewed changes

Chachay added 3 commits March 24, 2020 16:33

rename yaw_r to yaw_ref

6d178e1

rename yaw_r to yaw_ref

9b22d3f

rename a variable

b6e89c6

AtsushiSakai approved these changes Mar 26, 2020

View reviewed changes

AtsushiSakai merged commit 9274aac into AtsushiSakai:master Mar 26, 2020

Chachay deleted the MPC branch March 29, 2020 20:14

Uh oh!

[Proposal] Reduce internal dependancy #303

[Proposal] Reduce internal dependancy #303

Uh oh!

Conversation

Chachay commented Mar 20, 2020

Uh oh!

lgtm-com bot commented Mar 20, 2020

Uh oh!

AtsushiSakai commented Mar 23, 2020

Uh oh!

AtsushiSakai commented Mar 23, 2020

Uh oh!

AtsushiSakai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chachay commented Mar 24, 2020

Uh oh!

AtsushiSakai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AtsushiSakai Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AtsushiSakai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AtsushiSakai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AtsushiSakai Mar 25, 2020 •

edited

Loading

AtsushiSakai left a comment •

edited

Loading