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

Add in classical cv #204

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Add in classical cv #204

wants to merge 27 commits into from

Conversation

AshishA26
Copy link
Contributor

@AshishA26 AshishA26 commented Sep 29, 2024

Do not review. Waiting for tests.

@AshishA26 AshishA26 force-pushed the add_in_classical_cv branch from 5da7a5e to 6bb42a3 Compare October 2, 2024 18:35
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.

@achitaan achitaan force-pushed the add_in_classical_cv branch from 67691e0 to b1f0e49 Compare December 23, 2024 02:54
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 partially.

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

@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

@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

@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

@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.

Additional review.

def __init__(self, image: np.ndarray, boxes_list: np.ndarray):
"""
image = A numpy array that represents the image needed to be tested.
bounding_box_list: A numpy array that holds a list of expected bounding box coordinates.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Me as a new member making an easy mistake:

top_left_x = boxes_list[0]
top_left_y = boxes_list[1]

bottom_right_x = boxes_list[2]
bottom_right_x = boxes_list[3]

Add an explanation for the shape!

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.

Comment on lines 94 to 197
@pytest.fixture()
def image_easy(single_circle: InputImageAndExpectedBoundingBoxes) -> image_and_time.ImageAndTime: # type: ignore
"""
Load the single basic landing pad.
"""

image = single_circle.image
result, actual_image = image_and_time.ImageAndTime.create(image)
assert result
assert actual_image is not None
yield actual_image # type: ignore


@pytest.fixture()
def blurry_image(single_blurry_circle: InputImageAndExpectedBoundingBoxes) -> image_and_time.ImageAndTime: # type: ignore
"""
Load the single blurry landing pad.
"""

image = single_blurry_circle.image
result, actual_image = image_and_time.ImageAndTime.create(image)
assert result
assert actual_image is not None
yield actual_image # type: ignore


@pytest.fixture()
def stretched_image(single_stretched_circle: InputImageAndExpectedBoundingBoxes) -> image_and_time.ImageAndTime: # type: ignore
"""
Load the single stretched landing pad.
"""

image = single_stretched_circle.image
result, actual_image = image_and_time.ImageAndTime.create(image)
assert result
assert actual_image is not None
yield actual_image # type: ignore


@pytest.fixture()
def multiple_images(multiple_circles: InputImageAndExpectedBoundingBoxes) -> image_and_time.ImageAndTime: # type: ignore
"""
Load the multiple landing pads.
"""

image = multiple_circles.image
result, actual_image = image_and_time.ImageAndTime.create(image)
assert result
assert actual_image is not None
yield actual_image # type: ignore


@pytest.fixture()
def expected_easy(single_circle: InputImageAndExpectedBoundingBoxes) -> image_and_time.ImageAndTime: # type: ignore
"""
Load expected a basic image detections.
"""

expected = single_circle.bounding_box_list
yield create_detections(expected) # type: ignore


@pytest.fixture()
def expected_blur(single_blurry_circle: InputImageAndExpectedBoundingBoxes) -> image_and_time.ImageAndTime: # type: ignore
"""
Load expected the blured pad image detections.
"""

expected = single_blurry_circle.bounding_box_list
yield create_detections(expected) # type: ignore


@pytest.fixture()
def expected_stretch(single_stretched_circle: InputImageAndExpectedBoundingBoxes) -> image_and_time.ImageAndTime: # type: ignore
"""
Load expected a stretched pad image detections.
"""

expected = single_stretched_circle.bounding_box_list
yield create_detections(expected) # type: ignore


@pytest.fixture()
def expected_multiple(multiple_circles: InputImageAndExpectedBoundingBoxes) -> image_and_time.ImageAndTime: # type: ignore
"""
Load expected multiple pads image detections.
"""

expected = multiple_circles.bounding_box_list
yield create_detections(expected) # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are there so many fixtures doing the same work? I expect InputImageAndExpectedBoundingBoxes to already have the required information. Move this work into generate_detect_target_contour.py .

Copy link

Choose a reason for hiding this comment

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

I based a lot of my code on the ultralytics one so I thought this would be proper structure, should I do something else or just have it moved?

Copy link
Collaborator

@Xierumeng Xierumeng Mar 7, 2025

Choose a reason for hiding this comment

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

image_and_time.ImageAndTime.create() should already be part of the generation that results in InputImageAndExpectedBoundingBoxes .

create_detections() is not necessary because it just holds the time data (which is thrown away). compare_detections() should take in just the bounding boxes.

yield create_detections(expected) # type: ignore


# pylint:disable=duplicate-code
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove.

Copy link

Choose a reason for hiding this comment

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

Pylint fails without this though

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pylint is part of the coding standard, so suppressing it needs to be justified. Can you explain:

  • What exactly is causing this
  • Why it's not possible to change the code so that it no longer warns on this

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.


(x, y), radius = cv2.minEnclosingCircle(contour)

enclosing_area = np.pi * (radius**2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add spaces around **

expected_blur: detections_and_time.DetectionsAndTime,
) -> None:
"""
Run the detection for the blury cicular circle.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Spelling: for a single blurry circular landing pad.

@achitaan achitaan force-pushed the add_in_classical_cv branch from 03d95cb to 8ce749c Compare March 12, 2025 02:59
Comment on lines 181 to 186
# Create new object with actual detections
test_data = generate_detect_target_contour.InputImageAndTimeAndExpectedBoundingBoxes(
actual,
single_circle.bounding_box_list
)
compare_detections(test_data)
Copy link
Collaborator

@Xierumeng Xierumeng Mar 18, 2025

Choose a reason for hiding this comment

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

No!

def compare_detections(actual: list[Detection], expected: np.ndarray) -> None:
    ...

# The test
...
assert actual is not None

compare_detections(actual.detections, single_circle.bounding_box_list)
# End of test no more code here in this test

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.

None yet

3 participants