Remove api layers by mahircg · Pull Request #108 · IntelLabs/vdms

mahircg · 2019-05-20T15:37:26Z

No description provided.

mahircg · 2019-05-20T15:39:06Z

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.

luisremis

lgtm! Philip we will take a look and we are good to merge.
All tests passing in my system.

src/vcl/Image.cc

include/vcl/Video.h

philiplantz · 2019-06-20T01:57:58Z

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 · 2019-06-20T09:59:01Z

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 · 2019-06-20T19:24:01Z

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 e8cd292<e8cd292> mentions commit ea80b48<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 — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub<#108?email_source=notifications&email_token=AI2SLBADI43QYMX6OP2K2LDP3NIGNA5CNFSM4HODFKRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYE542Q#issuecomment-503963242>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AI2SLBECNWK5Z3JWSL4BWJDP3NIGNANCNFSM4HODFKRA>.

vishakha041 · 2019-06-24T18:15:49Z

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 · 2019-06-24T18:22:04Z

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

include/vcl/Image.h

SConstruct

include/vcl/Image.h

src/vcl/Image.cc

tests/SConscript

tests/unit_tests/Image_test.cc

include/vcl/Video.h

tests/SConscript

philiplantz · 2019-06-26T22:54:53Z

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 · 2019-06-27T00:47:32Z

I can't respond to this directly for some reason: #108 (comment)

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 · 2019-06-28T06:36:09Z

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

All comments addressed, looks good to go to me!

vishakha041 · 2019-07-01T23:44:16Z

SConstruct

                  'src/vcl/Image.cc',
                  'src/vcl/TDBImage.cc',
                  'src/vcl/Video.cc',
-                  'src/vcl/VideoData.cc',


Did you not face the VideoData test problem here?

Nope, because video testing is more on interface methods only. We can add more testing in the future of course.

vishakha041 · 2019-07-01T23:45:22Z

tests/SConscript

                'unit_tests/TDBImage_test.cc',
                'unit_tests/Image_test.cc',
                'unit_tests/Video_test.cc',
-                'unit_tests/VideoData_test.cc',


The commit message here still has a commit Id

vishakha041

Good sans two minor things. It would be nice to separate out operations next.

Unit test for ImageData class is removed, as the class itself will be refactored and merged into the Image class. Signed-off-by: mahircg <mahircan.guel@intel.com>

This patch consolidates ImageData and Image classes into the Image class, and removes ImageData class. Signed-off-by: Mahircan Guel <mahircan.guel@intel.com>

Currently, move constructor of Image class calls the copy-constructor. This causes creating redundant copies of move constructor's rhs argument, particularly for _operations vector and TDBImage object. This patch eliminates copying by calling std::move on operations, and assigning _tdb pointer without copying the content. Signed-off-by: mahircg <mahircan.guel@intel.com>

ImageData and Image classes are merged into a single class in a prior commit. Similarly, this patch moves the tests from ImageData_test class to Image_test class. Number of total tests are reduced from 60 (#ImageClass_test + #Image_test) to 53. This is due to duplicate tests that are removed. Below is a list of tests that are removed, and the tests they're replaced with. ====================================================== | Removed | Replaced with | ====================================================== |ImageData_test | Image_test | ------------------------------------------------------ |GetImageIDandImageFormat | GetMatFromTDB | |GetImageDimensions | GetImageDimensions | |GetEncodedBuffer | EncodedImage | |CreateUnique | CreateNamePNG | ====================================================== The remaining 3 tests had the same name in both files, so they're not shown in the list. Signed-off-by: mahircg <mahircan.guel@intel.com>

VideoData class was used as a wrapper around Video class which was maintained in a seperate repository, but not any longer. Since Video class is part of the VDMS codebase, abstraction layer provided by VideoData is redundant. This patch combines VideoData and Video classes into the Video class to remove the interface layer in between. Signed-off-by: mahircg <mahircan.guel@intel.com>

Since VideoData and Video classes are merged into a single class in a prior commit, testing a single module by two different unit tests is redundant. This patch moves the tests from VideoData_test class into Video_test class. Unit tests of Video class are either complementary or duplicate across the two test classes. Thus, the tests are mainly moved from VideoData_test to Video_test. Signed-off-by: mahircg <mahircan.guel@intel.com>

luisremis · 2019-07-02T17:45:33Z

Fixed the last commit message. Good to go!

* Add files to update pypi package

mahircg requested a review from luisremis May 20, 2019 15:37

luisremis requested review from philiplantz and vishakha041 May 20, 2019 17:27

luisremis previously approved these changes Jun 7, 2019

View reviewed changes

src/vcl/Image.cc Outdated Show resolved Hide resolved

include/vcl/Video.h Outdated Show resolved Hide resolved

mahircg dismissed luisremis’s stale review via 704db7e June 7, 2019 15:26

mahircg force-pushed the remove_api_layers branch 2 times, most recently from 704db7e to df9177e Compare June 14, 2019 08:06

philiplantz reviewed Jun 25, 2019

View reviewed changes

include/vcl/Image.h Outdated Show resolved Hide resolved

include/vcl/Image.h Outdated Show resolved Hide resolved

include/vcl/Image.h Outdated Show resolved Hide resolved

include/vcl/Image.h Outdated Show resolved Hide resolved

include/vcl/Image.h Outdated Show resolved Hide resolved

vishakha041 suggested changes Jun 25, 2019

View reviewed changes

mahircg force-pushed the remove_api_layers branch from df9177e to b0398bb Compare June 26, 2019 22:17

luisremis previously approved these changes Jun 28, 2019

View reviewed changes

vishakha041 reviewed Jul 1, 2019

View reviewed changes

vishakha041 previously approved these changes Jul 1, 2019

View reviewed changes

mahircg added 6 commits July 2, 2019 10:38

Remove ImageData_test unit-test

22ef435

Unit test for ImageData class is removed, as the class itself will be refactored and merged into the Image class. Signed-off-by: mahircg <mahircan.guel@intel.com>

Merge ImageData and Image classes

83caf9c

This patch consolidates ImageData and Image classes into the Image class, and removes ImageData class. Signed-off-by: Mahircan Guel <mahircan.guel@intel.com>

luisremis dismissed vishakha041’s stale review via 3b790be July 2, 2019 17:44

luisremis dismissed their stale review via 3b790be July 2, 2019 17:44

luisremis force-pushed the remove_api_layers branch from b0398bb to 3b790be Compare July 2, 2019 17:44

luisremis requested review from luisremis and philiplantz July 2, 2019 17:45

luisremis approved these changes Jul 2, 2019

View reviewed changes

luisremis merged commit 3623857 into develop Jul 2, 2019

luisremis deleted the remove_api_layers branch July 2, 2019 20:33

cwlacewe added a commit to cwlacewe/vdms that referenced this pull request Apr 14, 2023

Update Pypi for v2.4.0 (IntelLabs#108)

12dd421

* Add files to update pypi package

Conversation

mahircg commented May 20, 2019

Uh oh!

mahircg commented May 20, 2019

Uh oh!

luisremis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

philiplantz commented Jun 20, 2019

Uh oh!

mahircg commented Jun 20, 2019

Uh oh!

philiplantz commented Jun 20, 2019 via email

Uh oh!

vishakha041 commented Jun 24, 2019

Uh oh!

mahircg commented Jun 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philiplantz commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luisremis commented Jun 27, 2019

Uh oh!

vishakha041 commented Jun 28, 2019

Uh oh!

luisremis left a comment

Choose a reason for hiding this comment

Uh oh!

vishakha041 Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

luisremis Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishakha041 Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

vishakha041 left a comment

Choose a reason for hiding this comment

Uh oh!

luisremis commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

luisremis left a comment •

edited

Loading

philiplantz commented Jun 26, 2019 •

edited

Loading

luisremis Jul 2, 2019 •

edited

Loading