Remove api layers

[mahircg]

without_video_merge.log with_image_merge.log with_video_merge.log without_image_merge.log

I'm also attaching the logs for unit-test execution before and after merging the classes.

[philiplantz]

The commit message for commit e8cd292 mentions commit ea80b48, which has changed (and will presumably change again). Perhaps it would be better to remove the commit id from the message, and say "a prior commit".

[mahircg]

The commit message for commit e8cd292 mentions commit ea80b48, which has changed (and will presumably change again). Perhaps it would be better to remove the commit id from the message, and say "a prior commit".

Agreed, I'll edit it

[philiplantz]

Thanks. I suggest you wait until Vishakha and I have finished our reviews before you make any changes.

Philip

From: Mahircan Gül [mailto:notifications@github.com] Sent: Thursday, June 20, 2019 2:59 AM To: IntelLabs/vdms vdms@noreply.github.com Cc: Lantz, Philip philip.lantz@intel.com; Review requested review_requested@noreply.github.com Subject: Re: [IntelLabs/vdms] Remove api layers (#108)

The commit message for commit e8cd292https://github.com/IntelLabs/vdms/commit/e8cd29295dc4bad46c03721e36841a814f0ba61e mentions commit ea80b48https://github.com/IntelLabs/vdms/commit/ea80b48158728f935893b6734e2f3eb12f470294, which has changed (and will presumably change again). Perhaps it would be better to remove the commit id from the message, and say "a prior commit".

Agreed, I'll edit it

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHubhttps://github.com/IntelLabs/vdms/pull/108?email_source=notifications&email_token=AI2SLBADI43QYMX6OP2K2LDP3NIGNA5CNFSM4HODFKRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYE542Q#issuecomment-503963242, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AI2SLBECNWK5Z3JWSL4BWJDP3NIGNANCNFSM4HODFKRA.

[vishakha041]

Are these all the latest commits or are there pending changes based on Luis/Philip's comments? Just want to make sure I review the latest code. Thanks.

[mahircg]

Are these all the latest commits or are there pending changes based on Luis/Philip's comments? Just want to make sure I review the latest code. Thanks.

I haven't done the changes pointed out by Luis/Philip yet. Since those are rather small changes, the code won't change drastically once the changes are applied. I'd say it's good to go with the review.

Thanks

[philiplantz]

Vishakha and I had both commented that any API changes to the Image class should be separate from the Image/ImageData merge. But I had missed the fact that the body of the ImageData constructor was incorporated into Image as part of the merge and that the ImageData constructor already supported deep/shallow copy, so that capability naturally gets pulled in to Image as part of the merge. The only other API change was the addition of the default constructor, which has been made private, so it isn't actually an API change. So I think I'm now okay with not making a separate commit for API changes.

[luisremis]

I can't respond to this directly for some reason: https://github.com/IntelLabs/vdms/pull/108#discussion_r297830471

We will do a separate patch for that, not really a good reason not to other than video and images are high priority now. I will remove the separation on the feature vector implementation later and also add fixes that I have in my local branches as well.

[vishakha041]

I don't think github will send an email if you update the branch here. So please let us know whenever you are done so we can approve the PR. It should be good since you have applied all the changes.

[luisremis]

Fixed the last commit message. Good to go!