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

AICORE-22: Google Vision #267

Merged
merged 5 commits into from Oct 28, 2020
Merged

Conversation

vpasquier
Copy link
Contributor

No description provided.

@nuxeojenkins
Copy link
Contributor

View issue in JIRA: AICORE-22: Google Vision Enrichments Priority

@nuxeo-ai-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-ai-pr-267 here

@andreinechaev andreinechaev force-pushed the feature-AICORE-22-google-vision branch 3 times, most recently from 9ab09e3 to 81a3fc0 Compare October 26, 2020 16:27
@nuxeo-ai-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-ai-pr-267 here

1 similar comment
@nuxeo-ai-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-ai-pr-267 here

Copy link
Contributor

@PedroCardoso PedroCardoso left a comment

Choose a reason for hiding this comment

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

I do not see any test on the bounding box values. Maybe add a few. otherwise good

Comment on lines +40 to +42
if (vertices.size() != 4) {
return new AIMetadata.Box(0, 0, 0, 0);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is not exactly what we want.
I would go for the smallest box we could do to fit the complete polygon. Maybe this is too much for the current task, but create a ticket for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay. I think it makes sense. thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, as a second thought. If we don't have coordinates and those boxes are coming from multi object detection they will end up in one corner which doesn't make sense. Second, this case should never happen, I just decided that it's better than throwing an exception.

/**
* Marker interface for Enrichments with bounding box available
*/
public interface Polygonal {
Copy link
Contributor

Choose a reason for hiding this comment

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

Opening a can of worms ... but not sure why, but I don't like the name. It is not so much about polygon, maybe polygonToBox ! anyway, ignore this if you want :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't use transitive forms in naming :) A method inside may be named toBox or toCircle, the interface name should describe the object

@vpasquier
Copy link
Contributor Author

+1

@nuxeo-ai-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-ai-pr-267 here

@andreinechaev andreinechaev merged commit 95abc8e into master-10.10 Oct 28, 2020
@andreinechaev andreinechaev deleted the feature-AICORE-22-google-vision branch October 28, 2020 14:27
andreinechaev pushed a commit that referenced this pull request Oct 28, 2020
* AICORE-22: GCP enrichment providers

* AICORE-24: Add Google Vision Client Service

Co-authored-by: Andrei Nechaev <anechaev@nuxeo.com>
andreinechaev pushed a commit that referenced this pull request Oct 28, 2020
* AICORE-22: Google Vision (#267)

* AICORE-22: GCP enrichment providers

* AICORE-24: Add Google Vision Client Service

Co-authored-by: Andrei Nechaev <anechaev@nuxeo.com>

* AICORE-22: align on 11.x

Co-authored-by: Vladimir Pasquier <vpasquier@nuxeo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants