Skip to content
This repository was archived by the owner on Nov 13, 2025. It is now read-only.

Conversation

@vichua2006
Copy link
Contributor

No description provided.

Copy link
Contributor

@HermanG05 HermanG05 left a comment

Choose a reason for hiding this comment

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

Reviewed

Comment on lines 139 to 142
threshold_used, min_filtered_image = cv2.threshold(
grey_image, self.__config.min_brightness_threshold, 255, cv2.THRESH_TOZERO
)

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a good idea to add a check after this to see whether the method was successful before continuing

Copy link
Member

Choose a reason for hiding this comment

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

Just a quick question, why are you using THRESH_TOZERO while the original one uses THRESH_BINARY? Is there a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be a good idea to add a check after this to see whether the method was successful before continuing

Roger that, will do the error check.

Just a quick question, why are you using THRESH_TOZERO while the original one uses THRESH_BINARY? Is there a difference?

I was looking at docs, and it seems that THRESH_BINARY sets all values greater than the threshold to a specified maximum value (255 in this case). I thought that would defeat the purpose of the percentile filter afterwards, since whatever passed the min brightness would be set to 255 and automatically pass the percentile filter as well, regardless of its original value. THRESH_TOZERO sets values lower than the threshold to 0, but keeps values above the threshold as they are, and allows them to be filtered again by the percentile filter.
image

but now that I'm looking at the filters again, maybe it doesn't matter too much if the percentile filter is set to 99.0.

Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Reviewed

Comment on lines 139 to 142
threshold_used, min_filtered_image = cv2.threshold(
grey_image, self.__config.min_brightness_threshold, 255, cv2.THRESH_TOZERO
)

Copy link
Member

Choose a reason for hiding this comment

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

Just a quick question, why are you using THRESH_TOZERO while the original one uses THRESH_BINARY? Is there a difference?

# filter_by_area: True
# min_area_pixels: 50
# max_area_pixels: 640
# min_brightness_threshold: 50
Copy link
Member

Choose a reason for hiding this comment

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

please make this a lot higher, like at least 200, probably 225

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please make this a lot higher, like at least 200, probably 225

I originally had it at 200, but was failing the unit and the integration tests. It was able to detect the brightspots in the integration test at 100, and passed the unit test at 50. What should I do?

Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Reviewed

filter_by_area: Whether to filter by area.
min_area_pixels: Minimum area in pixels.
max_area_pixels: Maximum area in pixels.
min_brightness_threshold: Minimum brightness threshold for bright spots.
Copy link
Member

Choose a reason for hiding this comment

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

min_brightness_threshold: Minimum absolute brightness of individual pixels to be considered
min_average_brightness_threshold: Minimum absolute average brightness of detected blobs

Comment on lines 156 to 158
# cv2.imshow("Thresholded", bw_image) # type: ignore
# cv2.waitKey(0) # type: ignore

Copy link
Member

Choose a reason for hiding this comment

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

Delete

)
if threshold_used == 0:
self.__local_logger.error(f"{time.time()}: Failed to threshold image.")
self.__local_logger.error(f"{time.time()}: Failed to percentile threshold image.")
Copy link
Member

Choose a reason for hiding this comment

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

Don't print time.time(), just "Failed to percentile threshold image"

if len(keypoints) == 0:
self.__local_logger.info(f"{time.time()}: No brightspots detected.")
self.__local_logger.info(
f"{time.time()}: No brightspots detected (before blob average filter)."
Copy link
Member

Choose a reason for hiding this comment

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

Don't print time.time()

# A lack of detections is not an error, but should still not be forwarded
if len(filtered_keypoints) == 0:
self.__local_logger.info(
f"{time.time()}: No brightspots detected (after blob average filter)."
Copy link
Member

Choose a reason for hiding this comment

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

Don't print time.time()

Comment on lines 229 to 232
# Annotate the image (green circle) with detected keypoints
image_annotated = cv2.drawKeypoints(
image, keypoints, None, (0, 255, 0), cv2.DRAW_MATCHES_FLAGS_DRAW_RICH_KEYPOINTS
image, filtered_keypoints, None, (0, 255, 0), cv2.DRAW_MATCHES_FLAGS_DRAW_RICH_KEYPOINTS
)
Copy link
Member

Choose a reason for hiding this comment

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

Can we move all of this inside of the if statement on line 269 (only annotate images if needed)

	- deleted uncessary comments
	- slight annotation refactor
	- set integration test back to not saving image
Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Approved for temporary Flight test integration integration

@vichua2006 vichua2006 merged commit 030287d into main Mar 20, 2025
1 check failed
@vichua2006 vichua2006 deleted the min_brightness_filter branch March 20, 2025 15:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants