amazon-archives / aws-robomaker-sample-application-persondetection

Use AWS RoboMaker and demonstrate the use of Amazon Rekognition to recognize people's faces and Amazon Polly to synthesize speech.
MIT No Attribution
25 stars 18 forks source link

Log-based simulation regression test for AWS RoboMaker #45

Closed samuelgundry closed 4 years ago

samuelgundry commented 4 years ago

Using rosbag play, we are testing Person Detection recognizes faces from a recorded bag file.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

samuelgundry commented 4 years ago

my initial nits would be concerning missing documentation.

@dabonnie please be specific with missing documentation -- what are you looking for?

xabxx commented 4 years ago

Pylint has a few complaints per log_based_simulation_ws/src/person_detection_log_based_simulation/script/regression_test.py , my initial nits would be concerning missing documentation.

what documentation?

dabonnie commented 4 years ago

my initial nits would be concerning missing documentation.

@dabonnie please be specific with missing documentation -- what are you looking for?

Here's the full output from pylint (ignore the import issues, because that's specific to my system). Check out how it notes missing documentation via line number.

pylint regression_test.py No config file found, using default configuration ***** Module regression_test C: 27, 0: Line too long (109/100) (line-too-long) C: 59, 0: Trailing whitespace (trailing-whitespace) C: 61, 0: Line too long (112/100) (line-too-long) C: 64, 0: Trailing whitespace (trailing-whitespace) C: 76, 0: Trailing whitespace (trailing-whitespace) C: 77, 0: Trailing whitespace (trailing-whitespace) C: 89, 0: Line too long (149/100) (line-too-long) C: 1, 0: Missing module docstring (missing-docstring) E: 5, 0: Unable to import 'rospy' (import-error) E: 6, 0: Unable to import 'std_msgs.msg' (import-error) E: 7, 0: Unable to import 'robomaker_simulation_msgs.msg' (import-error) E: 8, 0: Unable to import 'robomaker_simulation_msgs.srv' (import-error) C: 15, 0: Missing function docstring (missing-docstring) C: 16, 4: Variable name "requestCancel" doesn't conform to snake_case naming style (invalid-name) W: 19, 8: Using the global statement (global-statement) C: 28, 4: Variable name "requestAddTags" doesn't conform to snake_case naming style (invalid-name) W: 36, 4: Using the global statement (global-statement) C: 47, 0: Missing function docstring (missing-docstring) W: 47,17: Unused argument 'timer' (unused-argument) C: 58, 0: Missing function docstring (missing-docstring) W: 59, 4: Using global for 'TOPIC' but no assignment is done (global-variable-not-assigned) C: 82, 0: Missing function docstring (missing-docstring) W: 87, 4: Using the global statement at the module level (global-at-module-level) C: 88, 4: Constant name "parser" doesn't conform to UPPER_CASE naming style (invalid-name) C: 90, 4: Constant name "args" doesn't conform to UPPER_CASE naming style (invalid-name) W: 8, 0: Unused ListTags imported from robomaker_simulation_msgs.srv (unused-import)


Your code has been rated at 2.76/10

Otherwise we could try using roslint: http://wiki.ros.org/roslint..?

samuelgundry commented 4 years ago

@dabonnie I've fixed some pylint. I'll leave these for now -- the two imports are false positives.

C:  1, 0: Missing module docstring (missing-docstring)
E:  7, 0: Unable to import 'robomaker_simulation_msgs.msg' (import-error)
E:  8, 0: Unable to import 'robomaker_simulation_msgs.srv' (import-error)
W: 20, 4: Using the global statement (global-statement)
W: 54,17: Unused argument 'timeout' (unused-argument)
C:106, 4: Invalid constant name "parser" (invalid-name)
C:109, 4: Invalid constant name "args" (invalid-name)
xabxx commented 4 years ago

some nitpicks, otherwise LGTM. SHIP IT! 😉 😊 😋 😎 😍

xabxx commented 4 years ago

Looks good. I can't approve as I'm the author. :)

:p waiting for any last comments if there is any. I will get this merged in by EOD.

AAlon commented 4 years ago

Closing this for now, when/if we want to get it in please reopen against the ros1 branch.