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

Put draw_box in image_processing #26712

Merged
merged 14 commits into from Sep 24, 2019

Conversation

@robmarkcole
Copy link
Contributor

commented Sep 19, 2019

Description:

Move shared draw_box() to platform level. Should this be tested? Type hints?

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]
@robmarkcole

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@robmarkcole robmarkcole requested a review from MartinHjelmare Sep 19, 2019
@pvizeli

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Type hints would be nice to understand what that is needed. You should also pin the library for drawing that to component level

@@ -64,6 +64,24 @@
PLATFORM_SCHEMA_BASE = cv.PLATFORM_SCHEMA_BASE.extend(PLATFORM_SCHEMA.schema)


def draw_box(draw, box, img_width, img_height, text="", color=(255, 255, 0)):

This comment has been minimized.

Copy link
@hunterjm

hunterjm Sep 19, 2019

Contributor

draw is an instance of ImageDraw from PIL, which isn't a hard dependency on the core Image Processing component. If we want to move this out of the individual integrations, it probably should be.

This comment has been minimized.

Copy link
@robmarkcole

robmarkcole Sep 19, 2019

Author Contributor

PIL itself provides a draw utility, is there a reason we don't use it? PIL.ImageDraw.Draw.rectangle

This comment has been minimized.

Copy link
@pvizeli

pvizeli Sep 19, 2019

Member

image processing is not a core integration, we can add it there to manifest...

This comment has been minimized.

Copy link
@pvizeli

pvizeli Sep 19, 2019

Member

future, you don't need to import the function. You can use hass.components.image_processing.draw_box()

@ivelin

This comment has been minimized.

Copy link

commented Sep 19, 2019

Thanks @robmarkcole for the tag. I like this PR. Its a step in the right direction. Thank you for putting in the effort.

A few other related topics that I am exploring (as mentioned 1:1) but do not affect this PR directly. I understand that @hunterjm might be the right person to get involved in the UI discussion related to AI image processing:

  • Users in the forums are asking for ability to review a timeline of images with detections. Not sure if there is a best practice in HASS for displaying a series of annotated images on demand. I see a few threads asking for image sliders. A social timeline type of card component could be applicable.
  • There are also frequent questions related to detection regions. Users don't want the AI to detect familiar objects that are of no interest security wise. It would be good to think of a way that allows users to draw on an image and share that feedback with the AI module so it can adjust its parameters. For example allow an SVG box or polygon draw layer over an image in HASS that is sent back to the AI component.
  • Pushing even further out on the wish list, there is interest in customizing the AI model by providing feedback for training. Allow users to adjust detection categories and boxes to help the AI adapt to the local environment.
@pvizeli

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

@ivelin the idea is to add an image to entity object with latest detection. We are not software for real image processing. But some simple things can help to adjust a platform.

@robmarkcole

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

We are basically very closely implementing tensorflows draw_bounding_boxes(). I took the liberty of fixing how the text is drawn as I noticed it was sometimes being covered by the box itself.

image

@@ -3,6 +3,9 @@
from datetime import timedelta
import logging

from typing import Tuple

This comment has been minimized.

Copy link
@pvizeli

pvizeli Sep 20, 2019

Member

Please follow this order or use isort

import CORE

import 3party

import ha

This comment has been minimized.

Copy link
@robmarkcole

robmarkcole Sep 21, 2019

Author Contributor

ok

@robmarkcole robmarkcole force-pushed the robmarkcole:refactor-image-processing branch 2 times, most recently from dbfa139 to e781c24 Sep 21, 2019
@robmarkcole

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

The tests are failing with ModuleNotFoundError: No module named 'PIL' although I have updated gen_requirements_all.py - what is going on here?

@robmarkcole

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

I am out of ideas on how to fix this

@MartinHjelmare MartinHjelmare moved this from Needs review to Review in progress in Dev Sep 22, 2019
@pvizeli

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

@robmarkcole you need run the generate script they update the requirements_all_test

@robmarkcole robmarkcole force-pushed the robmarkcole:refactor-image-processing branch from da53151 to 2d7df54 Sep 24, 2019
@robmarkcole

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Test passing, required pillow not Pillow...

Dev automation moved this from Review in progress to Reviewer approved Sep 24, 2019
Copy link
Member

left a comment

Maybe we can remove the Pillow pin on homeassistant.components.tensorflow?

@pvizeli

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

After that PR, we can add a function like media_player / cover

@MartinHjelmare MartinHjelmare removed their request for review Sep 24, 2019
robmarkcole added 13 commits Sep 20, 2019
@robmarkcole robmarkcole force-pushed the robmarkcole:refactor-image-processing branch from 8a57057 to 43d1c54 Sep 24, 2019
@pvizeli pvizeli merged commit 1d60ccc into home-assistant:dev Sep 24, 2019
11 checks passed
11 checks passed
CI Build #20190924.19 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 53e6b8a...43d1c54
Details
codecov/project 94.11% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 24, 2019
@lock lock bot locked and limited conversation to collaborators Sep 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
5 participants
You can’t perform that action at this time.