Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shamima/fall2020/recruitment-challenge/submission #4

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
# recruiting-challenges
Repository containing all paradigm student group recruiting challenges.
Repository containing paradigm student group recruiting challenges.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


59 changes: 52 additions & 7 deletions fall-2020/navigation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
"""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.desired_position = desired_position

pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done



Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def update_current_position(self):
""" Updates the current position of the TBM """
GPS.pollSensor
self.current_position = GPS.getPos
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are method calls, so the syntax will be:

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


pass


def navigate(self):
""" Navigate to the desired position from the current position

Based on the current position tuple, compared to the desired position tuple,
make decisions on steering, and ensure that the actuations are successful

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"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


""" These boolean values will indicate whether or not the actuations were suucessful."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment below navigate is a function docstring and should not have been removed, it acts as documentation for the function. It could have been modified, but the one you replaced it with doesn't describe the method.

It is good to include any assumptions in the docstring though (like preconditions).

Docstring notes/examples: https://www.python.org/dev/peps/pep-0257/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Successful*

act_request_forward = True
act_request_right = True
act_request_left = True
act_request_up = True
act_request_down = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good variable names



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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 4 quotations here instead of 3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

equal to the desired position in the x direction. """
if desired_position[0] != 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

while(current_position[0] != desired_position[0]):
act_request_forward = Steering.move_forward()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!



required_y_distance = desired_position[1] - current_position[1]
if required_y_distance > 0:
act_request_right = Steering.move_right(required_y_distance)
elif required_y_distance < 0:
act_request_left = Steering.move_left(required_y_distance)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if act_request_left == False or act_request_right == False:
print("Unable to move to desired y position")
self.update_current_position
return current_position == desired_position


required_z_distance = desired_position[2] - current_position[2]
if required_z_distance > 0:
act_request_up = Steering.move_up(required_z_distance)
elif required_z_distance < 0:
act_request_down = Steering.move_down(required_z_distance)
if act_request_up == False or act_request_down == False:
print("Unable to move to desired z position")
self.update_current_position
return current_position == desired_position

self.update_current_position
return current_position == desired_position

pass


Expand Down