Remove api layers #108

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:[email protected]] Sent: Thursday, June 20, 2019 2:59 AM To: IntelLabs/vdms <[email protected]> Cc: Lantz, Philip <[email protected]>; Review requested <[email protected]> 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

@@ -67,7 +67,6 @@ def buildServer(env):
                  '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

@@ -36,7 +36,6 @@ test_sources = ['main.cc',
                '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 <[email protected]>

This patch consolidates ImageData and Image classes into the Image class, and removes ImageData class. Signed-off-by: Mahircan Guel <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 vishakha041 and philiplantz 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 <[email protected]>

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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove api layers #108

Remove api layers #108

mahircg commented May 20, 2019

mahircg commented May 20, 2019

luisremis left a comment •

edited

Loading

philiplantz commented Jun 20, 2019

mahircg commented Jun 20, 2019

philiplantz commented Jun 20, 2019 via email

vishakha041 commented Jun 24, 2019

mahircg commented Jun 24, 2019

philiplantz commented Jun 26, 2019 •

edited

Loading

luisremis commented Jun 27, 2019

vishakha041 commented Jun 28, 2019

luisremis left a comment

vishakha041 Jul 1, 2019

luisremis Jul 2, 2019 •

edited

Loading

vishakha041 Jul 1, 2019

vishakha041 left a comment

luisremis commented Jul 2, 2019

Remove api layers #108

Remove api layers #108

Conversation

mahircg commented May 20, 2019

mahircg commented May 20, 2019

luisremis left a comment • edited Loading

Choose a reason for hiding this comment

philiplantz commented Jun 20, 2019

mahircg commented Jun 20, 2019

philiplantz commented Jun 20, 2019 via email

vishakha041 commented Jun 24, 2019

mahircg commented Jun 24, 2019

philiplantz commented Jun 26, 2019 • edited Loading

luisremis commented Jun 27, 2019

vishakha041 commented Jun 28, 2019

luisremis left a comment

Choose a reason for hiding this comment

vishakha041 Jul 1, 2019

Choose a reason for hiding this comment

luisremis Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

vishakha041 Jul 1, 2019

Choose a reason for hiding this comment

vishakha041 left a comment

Choose a reason for hiding this comment

luisremis commented Jul 2, 2019

luisremis left a comment •

edited

Loading

philiplantz commented Jun 26, 2019 •

edited

Loading

luisremis Jul 2, 2019 •

edited

Loading