Shamima/fall2020/recruitment-challenge/submission #4

Shamima-Ali · 2020-09-30T04:28:27Z

Implemented the Navigation class.

- def update_current_position(self)
Used the PollSensor method from GPS to update the x, y and z values. The getPos method is then used to return the x, y and z values to the Navigation class.

- def navigate(self)
Created 5 boolean variables that will store the boolean value returned from the Steering methods (move_up, move_down etc.).The boolean variables have been assigned a default value of True so if any of the Steering methods were unsuccessful, the variables will become False. If that happens, the TBM will stop moving and display an error message indicating which direction (x, y or z) it couldn't move to. The current_position will also be updated and compared to the desired_position.
------For moving in the x direction: As long as the desired_position is not equal to 0 or current_position, the TBM will move forwards.
------For moving in the y or z direction: The required y or z positions are found by subtracting the current_position and desired_position. If the required y or z directions are positive, the TBM will move to the right and upwards. It will move in the opposite directions if the required y or z positions are negative.
If all of the actuations are successful, the current_position will be updated and compared to the desired_position.

git status

colton-smith · 2020-10-25T20:27:11Z

So I don't forget, I'm gonna mention immediately that it would be a good idea to have the default "Actuation Successful" boolean values as False instead of True, for safety reasons.

colton-smith

Well done Shamima!

Git Practice:
Branch name - great, follows the requested format
PR Description - great, very descriptive. You could also mention the changes to the top level README.md just so that all your bases are covered and all files changed are noted.
PR Title - great, follows the requested format
Commit Messages - concise and descriptive

Code:
The code is very thorough and shows a lot of thought. There are a few minor things to address, and they are mostly style related.

Importantly:

Steering is a type, you want to make sure you are using an instance of the class and not the type
You are passing steer left and steer down negative values
Pass is a placeholder and should be removed when the method is implemented

Thanks for the submission, you can effect some changes and re-request review if you so desire!

colton-smith · 2020-10-25T20:29:41Z

README.md

-Repository containing all paradigm student group recruiting challenges.
+Repository containing paradigm student group recruiting challenges.


You're right, it does sound better without all.

I didn't really mean to make that correction😄 I was just trying out to 'commit' feature on my computer.

colton-smith · 2020-10-25T20:30:20Z

fall-2020/navigation.py

@@ -70,23 +70,68 @@ def set_desired_position(self, desired_position):
        Note: assume the user will pass in the desired_position parameter when using
        the interface 
        """
+
+        self.desired_position = desired_position
+
        pass


All instances of pass should be removed, they were only placed there as placeholders.

colton-smith · 2020-10-25T20:31:05Z

fall-2020/navigation.py

@@ -70,23 +70,68 @@ def set_desired_position(self, desired_position):
        Note: assume the user will pass in the desired_position parameter when using
        the interface 
        """
+


PEP8 - There should not be a blank line following any function/method docstring.

colton-smith · 2020-10-25T20:31:47Z

fall-2020/navigation.py

PEP8 - Python methods should only be separated by a single blank line.

colton-smith · 2020-10-25T20:32:33Z

fall-2020/navigation.py

+        act_request_down    = True
+
+
+        """"The TBM will move forward as long as the current position is not


There are 4 quotations here instead of 3

Also, # is preferred for block comments as opposed to """

colton-smith · 2020-10-25T20:39:51Z

fall-2020/navigation.py

+        act_request_forward = True
+        act_request_right   = True
+        act_request_left    = True
+        act_request_up      = True
+        act_request_down    = True


In python it is uncommon to align things like this, instead a space is usually placed before and after the assignment. This is almost assembly-like.

Ex:
number = 10
position = 11
velocity = 12

colton-smith · 2020-10-25T20:40:13Z

fall-2020/navigation.py

+        act_request_forward = True
+        act_request_right   = True
+        act_request_left    = True
+        act_request_up      = True
+        act_request_down    = True


Good variable names

colton-smith · 2020-10-25T20:42:40Z

fall-2020/navigation.py

+        equal to the desired position in the x direction. """
+        if desired_position[0] != 0:
+            while(current_position[0] != desired_position[0]):
+                act_request_forward = Steering.move_forward()


Steering is the class type, I think you mean to refer to the Steering instance that is a class field - "steering"

colton-smith · 2020-10-25T20:44:12Z

fall-2020/navigation.py

-        Returns: True if actuation requests were successful, False if not
-        Note: It may be good to notify the user if something unexpected happens!
-        """
+        """ Assumption: If any actuation is unsuccessful, the TBM will stop moving"""


I think this method is supposed to allow the calling code to stop the TBM in the event that an actuation is unsuccessful

colton-smith · 2020-10-25T20:47:28Z

fall-2020/navigation.py

+        elif required_y_distance < 0:
+            act_request_left = Steering.move_left(required_y_distance)


Here, the required distance is negative, indicating you have to move left. However, you are calling move left and passing it this negative value. Depending on the implementation of the move_left method, this could steer the machine right or do nothing. Maybe you can use the abs method to get the magnitude of required_distance.

This comment also applies to the z steering block.

Burke-Daniel

I've just included a couple of nit-picky comments here, Colton got all the major things I noticed as well. Keep up the good work!

Burke-Daniel · 2020-10-18T16:53:38Z

fall-2020/navigation.py

+        GPS.pollSensor
+        self.current_position = GPS.getPos


These are method calls, so the syntax will be:

GPS.pollSensor() self.current_position = GPS.getPos()

Burke-Daniel · 2020-10-18T16:57:51Z

fall-2020/navigation.py

+        equal to the desired position in the x direction. """
+        if desired_position[0] != 0:
+            while(current_position[0] != desired_position[0]):
+                act_request_forward = Steering.move_forward()


so you are calling the methods in Steering as if they were static methods. In order to call these, you need to call them from the steering object that is owned by the Navigation class, so this would instead look like:

act_request_forward = self.steering.move_forward()

Burke-Daniel · 2020-10-18T16:58:45Z

fall-2020/navigation.py

+                if act_request_forward == False:
+                    print("Unable to move to desired x position")
+                    self.update_current_position
+                    return current_position == desired_position


I like the way you have this, where if actuation fails it will return whether or not we have reached the desired position. Nifty way of doing it!

Burke-Daniel · 2020-10-18T17:04:19Z

fall-2020/navigation.py

+
+        """"The TBM will move forward as long as the current position is not
+        equal to the desired position in the x direction. """
+        if desired_position[0] != 0:


should be self.desired_position[0], as the desired_position variable is a member of the object

Thanks for taking the time to review the code. I made the changes as requested.

Shamima Ali and others added 5 commits September 26, 2020 00:55

Removed 'all' from README.md

dd53080

git status

Implemented set_desired_position

b496ed5

Implemented update_current_position

b76952a

Added code for navigate

1a6a117

Added comments

f822c5d

colton-smith requested changes Oct 25, 2020

View reviewed changes

Burke-Daniel requested changes Oct 26, 2020

View reviewed changes

Update navigation.py

37d7970

Thanks for taking the time to review the code. I made the changes as requested.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shamima/fall2020/recruitment-challenge/submission #4

Shamima/fall2020/recruitment-challenge/submission #4

Shamima-Ali commented Sep 30, 2020

colton-smith commented Oct 25, 2020

colton-smith left a comment

colton-smith Oct 25, 2020

Shamima-Ali Nov 4, 2020

colton-smith Oct 25, 2020

Shamima-Ali Nov 4, 2020

colton-smith Oct 25, 2020

colton-smith Oct 25, 2020

colton-smith Oct 25, 2020

colton-smith Oct 25, 2020

colton-smith Oct 25, 2020

colton-smith Oct 25, 2020

colton-smith Oct 25, 2020

colton-smith Oct 25, 2020

colton-smith Oct 25, 2020

Burke-Daniel left a comment

Burke-Daniel Oct 18, 2020

Burke-Daniel Oct 18, 2020

Burke-Daniel Oct 18, 2020

Burke-Daniel Oct 18, 2020

		Repository containing all paradigm student group recruiting challenges.
		Repository containing paradigm student group recruiting challenges.

		act_request_down = True


		""""The TBM will move forward as long as the current position is not

		elif required_y_distance < 0:
		act_request_left = Steering.move_left(required_y_distance)

Shamima/fall2020/recruitment-challenge/submission #4

Are you sure you want to change the base?

Shamima/fall2020/recruitment-challenge/submission #4

Conversation

Shamima-Ali commented Sep 30, 2020

colton-smith commented Oct 25, 2020

colton-smith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Burke-Daniel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment