-
Notifications
You must be signed in to change notification settings - Fork 39
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
Implement brightspot detection #228
Conversation
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.
Reviewed
# Object detections | ||
with open(filename + ".txt", "w", encoding="utf-8") as file: | ||
# Use internal string representation | ||
file.write(repr(detections)) |
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.
Don't log files anymore, look at the new DetectTargetUltralytics for the desired logging (You will need to insert a parameter for passing in a logger)
gray_image = cv2.cvtColor(image, cv2.COLOR_BGR2GRAY) | ||
|
||
# Apply thresholding to isolate bright spots | ||
_, bw_image = cv2.threshold(gray_image, BRIGHTSPOT_THRESHOLD, 255, cv2.THRESH_BINARY) |
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.
What is the first parameter? Why is it unused? If it tells apart success vs failure, please use an if or try/except and log error
Return: Success, detections. | ||
""" | ||
image = data.image | ||
gray_image = cv2.cvtColor(image, cv2.COLOR_BGR2GRAY) |
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.
put in try/except and logger.error any errors
from .. import detections_and_time | ||
|
||
|
||
BRIGHTSPOT_THRESHOLD = 240 |
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.
Could you make this a parameter that's passed in from config.yaml? It may be easier to test that way. For your unit tests you can continue to use a hard coded value. cc @Xierumeng do we want to modify detect target factory to have a dict (kind of like kwargs?) for these kinds of things?
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 dislike kwargs because it removes information from a function signature. Perhaps a data exchange format for the configs? For example:
detect_target_ultralytics.py
class DetectTargetUltralyticsConfig:
def __init__(self, thing, other_thing):
...
class DetectTargetUltralytics:
@classmethod
def create(cls, common_thing, another_common, config: DetectTargetUltralyticsConfig):
...
detect_target_brightspot.py
class DetectTargetBrightspotConfig:
def __init__(self, other_thing, setting):
...
class DetectTargetBrightspot:
@classmethod
def create(cls, common_thing, another_common, config: DetectTargetBrightspotConfig):
...
detect_target_factory.py
def create_detect_target(detect_target_option: DetectTargetOption, common_thing, another_common, config: DetectTargetUltralyticsConfig | DetectTargetBrightspotConfig):
match detect_target_option:
case DetectTargetOption.ML_ULTRALYTICS:
return detect_target_ultralytics.DetectTargetUltralytics.create(common_thing, another_common, config)
case DetectTargetOption.C_BRIGHTSPOT:
return detect_target_brightspot.DetectTargetBrightspot.create(common_thing, another_common, config)
return False, None
|
||
TEST_PATH = pathlib.Path("tests", "brightspot_example") | ||
|
||
IMAGE_IR1_PATH = pathlib.Path(TEST_PATH, "ir.png") |
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.
Maybe rename your photo to ir1.png?
size = keypoint.size | ||
bounds = np.array([x - size / 2, y - size / 2, x + size / 2, y + size / 2]) | ||
confidence = keypoint.response | ||
result, detection = detections_and_time.Detection.create(bounds, 1, confidence) |
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.
Rather than using magic number 1, set as enum or a global constant at the top. Also, do you know what the keypoint.response is? It seems to be 0, which usually means it is not confident?
Test detection on multiple copies of the IR image. | ||
""" | ||
image_count = 4 | ||
input_images = [copy.deepcopy(image_ir) for _ in range(image_count)] |
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 should test on some new images that we collected from flight test. See oneDrive, 11-23 and 11-22 (pick before/after tracks, around 100.png for ~e=250).
Also find a pic that has no IR beacon to make sure it detects nothing
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.
Reviewed functionality, tests to be reviewed.
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.
Reviewed tests.
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.
Checked types.
0fe3eac
to
8f18e9c
Compare
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.
Reviewed.
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.
Reviewed.
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.
Approved, thanks!
No description provided.