-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
dd53080
b496ed5
b76952a
1a6a117
f822c5d
37d7970
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
""" | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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/ There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are 4 quotations here instead of 3 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be |
||
while(current_position[0] != desired_position[0]): | ||
act_request_forward = Steering.move_forward() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
||
|
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.