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

main_2024.py detect target #156

Merged
merged 36 commits into from
Feb 17, 2024
Merged

main_2024.py detect target #156

merged 36 commits into from
Feb 17, 2024

Conversation

Ethan118
Copy link
Contributor

Checked detect target worker integration in main_2024.py.

What changed

  • Changed return type to Detections and Time
  • Added CLI option to display annotated image

Files changed

  • main_2024.py
  • detect_target.py
  • detect_target_worker.py

main_2024.py Outdated Show resolved Hide resolved
Comment on lines 86 to 89
if self.__show_annotations:
cv2.imshow("Annotated Image", image_annotated)

return True, detections
Copy link
Member

@Trotyl15 Trotyl15 Nov 28, 2023

Choose a reason for hiding this comment

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

Does it work on your end? I don't think cv2.imshow is thread safe. Maybe consider passing image_annotated to the main process and display it from there instead of in worker process or implement locks.

Copy link
Collaborator

@Xierumeng Xierumeng Nov 29, 2023

Choose a reason for hiding this comment

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

It probably isn't threadsafe, but since only a single process will be using it, it should be fine. The Autonomy bootcamp is an example of where it works without issue.

Copy link
Member

Choose a reason for hiding this comment

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

Did a little test and when cv2.imshow was placed in worker process, the window freezes at start up my machine. For where it's currently placed, it works fine. Not sure if it's the same case with @Ethan118

@Ethan118 Ethan118 requested a review from Trotyl15 November 28, 2023 22:05
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

modules/detect_target/detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
@Ethan118 Ethan118 requested a review from Xierumeng November 30, 2023 03:05
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert to when you were displaying the image in run() . Return only the detections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amy mentioned above that this causes issues. Is there a work around?

Copy link
Member

Choose a reason for hiding this comment

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

Does it work on you end? If it does then maybe I missed something in the set up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you try adding cv2.waitKey(1) after the imshow() ?

Copy link
Member

Choose a reason for hiding this comment

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

Can you try adding cv2.waitKey(1) after the imshow() ?

Oh this works! @Ethan118 you can do this instead.

tests/test_detect_target.py Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

main_2024.py Outdated Show resolved Hide resolved
main_2024.py Show resolved Hide resolved
main_2024.py Outdated
cv2.imshow("Landing Pad Detector", image)

if cv2.waitKey(1) & 0xFF == ord('q'):
if cv2.waitKey(1) & 0xFF == ord("q"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does pressing q work for exiting the program?

tests/model_example/generate_expected.py Show resolved Hide resolved
tests/model_example/generate_expected.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

tests/model_example/generate_expected.py Outdated Show resolved Hide resolved
tests/model_example/generate_expected.py Show resolved Hide resolved
tests/model_example/generate_expected.py Outdated Show resolved Hide resolved
tests/model_example/generate_expected.py Outdated Show resolved Hide resolved
tests/model_example/generate_expected.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

tests/model_example/generate_expected.py Outdated Show resolved Hide resolved
tests/model_example/generate_expected.py Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

tests/model_example/generate_expected.py Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed. The tolerances may need to be relaxed, since the unit test isn't passing on GitHub.

main_2024.py Outdated Show resolved Hide resolved
tests/model_example/generate_expected.py Outdated Show resolved Hide resolved
tests/model_example/generate_expected.py Outdated Show resolved Hide resolved
tests/model_example/generate_expected.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Outdated Show resolved Hide resolved
@Ethan118
Copy link
Contributor Author

Ethan118 commented Feb 7, 2024

Reviewed. The tolerances may need to be relaxed, since the unit test isn't passing on GitHub.

Ya it's weird because the tests pass on my computer. Maybe it has something to do with the override precision?

Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed.

tests/test_detect_target.py Outdated Show resolved Hide resolved
tests/test_detect_target.py Show resolved Hide resolved
tests/test_detect_target.py Show resolved Hide resolved
modules/detect_target/detect_target.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Reviewed. Does test_detect_target_worker.py pass as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove line 12 import (no longer being used).

main_2024.py Outdated
@@ -45,6 +45,7 @@ def main() -> int:
parser = argparse.ArgumentParser()
parser.add_argument("--cpu", action="store_true", help="option to force cpu")
parser.add_argument("--full", action="store_true", help="option to force full precision")
parser.add_argument("--show-annotated", action="store_true", help="option to show annotated image")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make multiline.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove line 7 import (no longer being used).

@@ -17,7 +17,7 @@ class DetectTarget:
"""
Contains the YOLOv8 model for prediction.
"""
def __init__(self, device: "str | int", model_path: str, override_full: bool, save_name: str = ""):
def __init__(self, device: "str | int", model_path: str, override_full: bool, show_annotations: bool = False, save_name: str = ""):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make multiline.

if override_full:
self.__enable_half_precision = False
self.__filename_prefix = ""
if save_name != "":
self.__filename_prefix = save_name + "_" + str(int(time.time())) + "_"

def run(self, data: image_and_time.ImageAndTime) -> "tuple[bool, np.ndarray | None]":
def run(self, data: image_and_time.ImageAndTime) -> "tuple[bool, detections_and_time.DetectionsAndTime | None]":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make multiline.

Comment on lines 40 to 45
np.testing.assert_almost_equal(expected_detection.confidence, actual_detection.confidence, decimal=CONFIDENCE_DECIMAL_TOLERANCE)

np.testing.assert_almost_equal(actual_detection.x1, expected_detection.x1, decimal=BOUNDING_BOX_DECIMAL_TOLERANCE)
np.testing.assert_almost_equal(actual_detection.y1, expected_detection.y1, decimal=BOUNDING_BOX_DECIMAL_TOLERANCE)
np.testing.assert_almost_equal(actual_detection.x2, expected_detection.x2, decimal=BOUNDING_BOX_DECIMAL_TOLERANCE)
np.testing.assert_almost_equal(actual_detection.y2, expected_detection.y2, decimal=BOUNDING_BOX_DECIMAL_TOLERANCE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make multiline.

assert detections is not None

for i in range(0, detections_from_file.shape[0]):
result, detection = detections_and_time.Detection.create(detections_from_file[i][2:], int(detections_from_file[i][1]), detections_from_file[i][0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make multiline or use intermediate variables.

"""
Load expected bus detections.
"""
expected_bus = np.loadtxt(BOUNDING_BOX_BUS_PATH)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename this to something that is not the function name (e.g. expected ).

# Test
np.testing.assert_almost_equal(actual_error, EXPECTED_ERROR)
@pytest.fixture()
def expected_zidane():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.


error = rmse(actual, expected)
assert error < self.__IMAGE_DIFFERENCE_TOLERANCE

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove unnecessary space characters.

Copy link
Collaborator

@Xierumeng Xierumeng left a comment

Choose a reason for hiding this comment

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

Approved.

@Ethan118 Ethan118 merged commit f7acbeb into main Feb 17, 2024
1 check passed
@Ethan118 Ethan118 deleted the detect_target-integration branch February 17, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants