Test refactor #91

drivanov · 2018-08-01T22:49:58Z

Refactoring of the unit tests

Elimination of duplicated code from WriteImage functions. Signed-off-by: Andrei <andreii@nvidia.com>

Signed-off-by: Andrei <andreii@nvidia.com>

The JPEG/PNG files are loaded and decoded only when it's is really need to be done for performing of current test. Signed-off-by: Andrei <andreii@nvidia.com>

Output of the images in benchmark eliminate Signed-off-by: Andrei <andreii@nvidia.com>

Signed-off-by: Andrei <andreii@nvidia.com>

cliffwoolley · 2018-08-01T23:01:59Z

dali/benchmark/resnet50_nvjpeg_bench.cc

@@ -112,7 +112,7 @@ BENCHMARK_DEFINE_F(RealRN50, nvjpegPipe)(benchmark::State& st) { // NOLINT
    }
  }

-  WriteHWCBatch<uint8_t>(*ws.Output<GPUBackend>(0), 0, 1, "img");
+//  WriteHWCBatch(*ws.Output<GPUBackend>(0), "img");


IIRC, the reason we keep this line here is for debugging, right? If so can we put it in #if DALI_DEBUG instead of commenting it out? Like was done below in dali/pipeline/operators/fused/crop_mirror_normalize_test.cc .

cliffwoolley · 2018-08-01T23:02:31Z

dali/benchmark/dali_bench.h

  }

  virtual ~DALIBenchmark() {
-    for (auto &ptr : jpegs_) {
+    for (auto &ptr : jpegs_.data_) {


Is it more future-proof to delete jpegs_ and count on its destructor to delete jpegs_.data_ in turn? That would mean ImgSetDescr in dali/util/image.h should be a class rather than a struct and have a non-default destructor (which is probably reasonable anyway).

Yes, it is a very useful change. It also fixes memory leak in DALISingleOpTest

- Destructor for ImageSetDescr - Conditional compilation of WriteHWCBatch for resnet50 benchmark Signed-off-by: Andrei <andreii@nvidia.com>

JanuszL · 2018-08-03T05:54:29Z

dali/pipeline/operators/decoder/host_decoder_test.cc

  }
 };

 typedef ::testing::Types<RGB, BGR, Gray> Types;
 TYPED_TEST_CASE(HostDecodeTest, Types);

 TYPED_TEST(HostDecodeTest, TestJPEGDecode) {
-  this->RunTestDecode(t_jpegImgType, 0.00000005);
+  this->RunTestDecode(t_jpegImgType, 1.6);


Why 1.6 and not 1.7 or 1.4?

Actually, on my machine all HostDecode tests are passing with 0.0 tolerance.
But in GitLab version the average deviation for different images is in following range
0.299477 - 1.57299. I am using 1.6 just to make these tests passed.

It is strange. As I see this test generates reference data using cv::imdecode, decoder itself should use jpeg turbo. Could you try investigate a bit more if this is jpeg turbo itself or other case condition?

I agree- it's very strange. I also see the discrepancy in JpegDecodeTest results. I will investigate.

Cliff helped me to investigate this issue. This is what we found by comparing the images constructed on my machine and during CI testing:
a) the images decoded by DALI are the same;
b) the images constructed by openCV methods are different.
c) the difference is not significant - the absolute values of the average deviation for different colored images are in range of (0.3 - 1.16), which is 0.11% -0.63% of the full color range of 255.

The real cause of this discrepancy is still unclear. I reinstalled openCV/JpegTurbo/NvJpeg and could not reproduce CI results.

@drivanov - I still recommend rescaling the results by 1/255.f and changing the threshold to match and being done with it.

OK, I will do that. It should not be difficult because in that branch the calculation of the average deviation is in one place for all tests which are using it.

Signed-off-by: Andrei <andreii@nvidia.com>

This class was used for - new tests for Displacement Operators. - refactoring of the tests for Color Operators Signed-off-by: Andrei <andreii@nvidia.com>

(normalized to color range and converted to percentages) Signed-off-by: Andrei <andreii@nvidia.com>

ptrendx · 2018-08-17T16:24:04Z

@cliffwoolley @drivanov What is the status of this PR? Can we review/merge it? I see that Andrei added commits from #93 here as well, which is confusing.

drivanov · 2018-08-17T17:14:42Z

@ptrendx Actually, #91 and #93 should be identical. Yesterday @cliffwoolley asked me to merge everything into #91 and I did it.

ptrendx · 2018-08-17T17:39:22Z

Ok, please close #93 then.

ptrendx · 2018-08-17T18:05:57Z

dali/pipeline/operators/operator_factory.h

@@ -37,10 +37,11 @@ class OperatorRegistry {

  OperatorRegistry() {}

-  void Register(const std::string &name, Creator creator) {
+  void Register(const std::string &name, Creator creator, const char *devName = NULL) {


const string &

And then use "" instead of NULL in allocator.h

ptrendx · 2018-08-17T18:08:00Z

dali/pipeline/operators/operator_factory.h

@@ -71,8 +72,8 @@ class Registerer {
 public:
  Registerer(const std::string &name,
      OperatorRegistry<OpType> *registry,
-      typename OperatorRegistry<OpType>::Creator creator) {
-    registry->Register(name, creator);
+      typename OperatorRegistry<OpType>::Creator creator, const char *devName = NULL) {


ptrendx · 2018-08-17T18:09:17Z

dali/pipeline/operators/resize/new_resize.h

@@ -143,7 +143,7 @@ class NewResize : public Resize<Backend> {
  void SetupSharedSampleParams(Workspace<Backend> *ws) override {
    Resize<Backend> ::SetupSharedSampleParams(ws);
  }
-  uint ResizeInfoNeeded() const override { return t_crop + t_mirrorHor; }
+  virtual uint ResizeInfoNeeded() const     { return t_crop + t_mirrorHor; }


Why? Override makes the code clearer.

ptrendx · 2018-08-17T18:12:22Z

dali/test/dali_test_single_op.h

+    RunOperator(AddArguments(&spec, descr.args), descr.epsVal);
+  }
+
+  void RunOperator(const OpSpec& spec, double eps, DeviceWorkspace *pWS = NULL) {


use nullptr instead of NULL

Signed-off-by: Andrei <andreii@nvidia.com>

* Division by 0 in in CropMirrorNormalizeTest was fixed. Elimination of duplicated code from WriteImage functions. Signed-off-by: Andrei <andreii@nvidia.com> * Fix of problem in bresnet50 benchmark Signed-off-by: Andrei <andreii@nvidia.com> * CropMirrorNormalizePermuteTest refactored Signed-off-by: Andrei <andreii@nvidia.com> * Lint problem fixed Signed-off-by: Andrei <andreii@nvidia.com> * JpegDecodeTest re-implemented Signed-off-by: Andrei <andreii@nvidia.com> * Nondefault bound for average deviation implemented for 2 decode tests Signed-off-by: Andrei <andreii@nvidia.com> * Executore test refactored. The JPEG/PNG files are loaded and decoded only when it's is really need to be done for performing of current test. Signed-off-by: Andrei <andreii@nvidia.com> * Lint problem fixed Output of the images in benchmark eliminate Signed-off-by: Andrei <andreii@nvidia.com> * Eps for ExecutorTest and some changes, suggested by Cliff Signed-off-by: Andrei <andreii@nvidia.com> * Changes requested by Cliff. - Destructor for ImageSetDescr - Conditional compilation of WriteHWCBatch for resnet50 benchmark Signed-off-by: Andrei <andreii@nvidia.com> * Color augmentations on CPU Signed-off-by: Andrei <andreii@nvidia.com> * GenericMatchingTest class implemented This class was used for - new tests for Displacement Operators. - refactoring of the tests for Color Operators Signed-off-by: Andrei <andreii@nvidia.com> * Average deviation of pixel colors for unit tests rescaled (normalized to color range and converted to percentages) Signed-off-by: Andrei <andreii@nvidia.com> * Changes requested by Przemek Signed-off-by: Andrei <andreii@nvidia.com> Signed-off-by: Andrei <andreii@nvidia.com>

drivanov added 9 commits August 1, 2018 15:48

Division by 0 in in CropMirrorNormalizeTest was fixed.

0fd1e10

Elimination of duplicated code from WriteImage functions. Signed-off-by: Andrei <andreii@nvidia.com>

Fix of problem in bresnet50 benchmark

87913e0

Signed-off-by: Andrei <andreii@nvidia.com>

CropMirrorNormalizePermuteTest refactored

23fe2a7

Signed-off-by: Andrei <andreii@nvidia.com>

Lint problem fixed

7d1dc18

Signed-off-by: Andrei <andreii@nvidia.com>

JpegDecodeTest re-implemented

f98b033

Signed-off-by: Andrei <andreii@nvidia.com>

Nondefault bound for average deviation implemented for 2 decode tests

6219cae

Signed-off-by: Andrei <andreii@nvidia.com>

Executore test refactored.

e302acd

The JPEG/PNG files are loaded and decoded only when it's is really need to be done for performing of current test. Signed-off-by: Andrei <andreii@nvidia.com>

Lint problem fixed

3361186

Output of the images in benchmark eliminate Signed-off-by: Andrei <andreii@nvidia.com>

Eps for ExecutorTest and some changes, suggested by Cliff

77214e9

Signed-off-by: Andrei <andreii@nvidia.com>

cliffwoolley requested changes Aug 1, 2018

View reviewed changes

Changes requested by Cliff.

21ee128

- Destructor for ImageSetDescr - Conditional compilation of WriteHWCBatch for resnet50 benchmark Signed-off-by: Andrei <andreii@nvidia.com>

drivanov force-pushed the test_refactor_GH branch from 3521365 to 21ee128 Compare August 2, 2018 18:14

cliffwoolley mentioned this pull request Aug 2, 2018

Color augmentations on CPU #93

Closed

cliffwoolley approved these changes Aug 2, 2018

View reviewed changes

cliffwoolley requested a review from JanuszL August 2, 2018 22:17

JanuszL reviewed Aug 3, 2018

View reviewed changes

drivanov added 3 commits August 16, 2018 09:44

Color augmentations on CPU

a68e9f1

Signed-off-by: Andrei <andreii@nvidia.com>

GenericMatchingTest class implemented

902bf8f

This class was used for - new tests for Displacement Operators. - refactoring of the tests for Color Operators Signed-off-by: Andrei <andreii@nvidia.com>

Average deviation of pixel colors for unit tests rescaled

c9ab895

(normalized to color range and converted to percentages) Signed-off-by: Andrei <andreii@nvidia.com>

ptrendx requested changes Aug 17, 2018

View reviewed changes

Changes requested by Przemek

194fd30

Signed-off-by: Andrei <andreii@nvidia.com>

ptrendx approved these changes Aug 17, 2018

View reviewed changes

ptrendx merged commit fb1d481 into NVIDIA:master Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test refactor #91

Test refactor #91

drivanov commented Aug 1, 2018

cliffwoolley Aug 1, 2018

drivanov Aug 2, 2018

cliffwoolley Aug 1, 2018

drivanov Aug 2, 2018

JanuszL Aug 3, 2018

drivanov Aug 6, 2018

JanuszL Aug 7, 2018

drivanov Aug 7, 2018

drivanov Aug 9, 2018

cliffwoolley Aug 10, 2018

drivanov Aug 10, 2018

ptrendx commented Aug 17, 2018

drivanov commented Aug 17, 2018

ptrendx commented Aug 17, 2018

ptrendx Aug 17, 2018

ptrendx Aug 17, 2018

ptrendx Aug 17, 2018

ptrendx Aug 17, 2018

ptrendx Aug 17, 2018

Test refactor #91

Test refactor #91

Conversation

drivanov commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptrendx commented Aug 17, 2018

drivanov commented Aug 17, 2018

ptrendx commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment