RPP Test Suite Upgrade - CSV to Binary Conversion #222

HazarathKumarM · 2024-01-24T19:50:02Z

merge golden outputs as a single binary file instead off multiple CSV files

r-abishek

@HazarathKumarM Please address comments.

r-abishek · 2024-01-24T21:06:19Z

utilities/test_suite/rpp_test_suite_common.h

+        outputTempRef = refOutput + imageCnt * refOutputSize;
+        int height = dstImgSizes[imageCnt].height;
+        int width = dstImgSizes[imageCnt].width;
+        int matched_idx = 0;


This variable alone is in snake case

r-abishek · 2024-01-24T21:11:37Z

utilities/test_suite/rpp_test_suite_common.h

+        std::cerr << "File is empty";
+
+    fseek(fp, 0, SEEK_SET);
+    Rpp8u *binary_content = (Rpp8u *)malloc(fsize);


Where is the free? Also please check if we already know the size 150x150x3 or something, can we allocate static?

for some cases we don't have PLN1 support , for that cases we will be allocating more memory if we follow the static allocation

Should be fine i feel
The size of 3x150x150x3 is less than a MB

r-abishek · 2024-01-24T21:13:09Z

utilities/test_suite/rpp_test_suite_common.h

-        compare_outputs_pkd(output, refOutput, dstDescPtr, dstImgSizes, refOutputHeight, refOutputWidth, refOutputSize, fileMatch);
+        compare_outputs_pkd(output, binary_content, dstDescPtr, dstImgSizes, refOutputHeight, refOutputWidth, refOutputSize, fileMatch);
+    else if(dstDescPtr->layout == RpptLayout::NCHW && dstDescPtr->c == 3)
+        compare_outputs_pln3(output, binary_content, dstDescPtr, dstImgSizes, refOutputHeight, refOutputWidth, refOutputSize, fileMatch);


camelCase consistency

r-abishek · 2024-01-24T21:14:54Z

utilities/test_suite/rpp_test_suite_common.h

+        std::cerr << "File is empty";
+
+    fseek(fp, 0, SEEK_SET);
+    Rpp64u *binary_content = (Rpp64u *)malloc(fsize);


r-abishek · 2024-01-24T21:16:59Z

utilities/test_suite/rpp_test_suite_common.h


    int fileMatch = 0;
    int matched_values = 0;
    if(srcDescPtr->c == 1)
    {
-        for(int i = 0; i < srcDescPtr->n; i++)
+        for(int i = 0, j = srcDescPtr->n * 4; i < srcDescPtr->n; i++,j++)


formatting - space after comma

r-abishek · 2024-01-24T21:17:59Z

utilities/test_suite/rpp_test_suite_voxel.h

+void compare_outputs_pln3(Rpp32f* output, Rpp32f* refOutput, int &fileMatch, RpptGenericDescPtr descriptorPtr3D, RpptRoiXyzwhd *roiGenericSrcPtr)
+{
+    Rpp32f *rowTemp, *rowTempRef, *outVal, *outRefVal, *outputTemp, *outputTempRef, *outputTempChn, *outputTempRefChn, *depthTemp, *depthTempRef;
+    for(int Cnt = 0; Cnt < descriptorPtr3D->dims[0]; Cnt++)


formatting - Small c.
cnt or count.

r-abishek · 2024-01-24T21:26:12Z

@sampath1117 Please do a round of review and ensure tests passing

sampath1117

@HazarathKumarM
Please address review comments

sampath1117 · 2024-01-25T07:32:01Z

utilities/test_suite/rpp_test_suite_common.h

@@ -950,7 +950,7 @@ void compare_outputs_pln(Rpp8u* output, Rpp8u* refOutput, RpptDescPtr dstDescPtr
        outputTempRef = refOutput + imageCnt * refOutputSize;
        int height = dstImgSizes[imageCnt].height;
        int width = dstImgSizes[imageCnt].width;
-        int matched_idx = 0;
+        int matchedIdx = 0;


Need not have the channel loop for PLN1 variant at L957, since now
PLN3 is decoupled

I think PKD3 and PLN1 can be combined in single function?
Please check

sampath1117 · 2024-01-25T07:34:09Z

utilities/test_suite/rpp_test_suite_common.h

+                for(int j = 0; j < width; j++)
+                {
+                    outVal = rowTemp + j;
+                    outRefVal = rowTempRef + j * 3 ;


Remove space before semicolon at L1004
Also better to add a comment that reference output is PLN3 and test suite output is in PKD3 for better clarity?

@r-abishek Please share your thoughts also on this

sampath1117 · 2024-01-25T07:35:08Z

utilities/test_suite/rpp_test_suite_common.h

@@ -997,6 +1034,8 @@ inline void compare_output(T* output, string funcName, RpptDescPtr srcDescPtr, R
        func += dataType[dstDescPtr->dataType];
    }

+    std::string binFile = func + "Tensor";
+


Remove the additional space at L1038

sampath1117 · 2024-01-25T07:37:34Z

utilities/test_suite/rpp_test_suite_common.h

-        return;
-    }
+    FILE *fp;
+    fp = fopen(refFile.c_str(), "rb");


Try adding a small helper function for reading the data from binary file, instead of readding same code

sampath1117 · 2024-01-25T07:38:30Z

utilities/test_suite/rpp_test_suite_common.h


    int fileMatch = 0;
    int matched_values = 0;
    if(srcDescPtr->c == 1)
    {
-        for(int i = 0; i < srcDescPtr->n; i++)
+        for(int i = 0, j = srcDescPtr->n * 4; i < srcDescPtr->n; i++, j++)


why do we need to add j counter in this loop?
It is only for PLN1 right

removed the j loop counter and updated the pointer instead

sampath1117 · 2024-01-25T07:39:38Z

utilities/test_suite/rpp_test_suite_common.h

        status += "PASSED";
    }
    else
    {
-        std::cerr << "FAILED! " << fileMatch << "/" << srcDescPtr->n << " outputs are matching with reference outputs" << std::endl;
+        std::cout << "FAILED! " << fileMatch << "/" << srcDescPtr->n << " outputs are matching with reference outputs" << std::endl;


is there any reason for changing cout to cerr?

If we use cerr the summary is printed before the cpp code printing time log to the console

sampath1117 · 2024-01-25T07:45:26Z

utilities/test_suite/rpp_test_suite_common.h

+        std::cerr << "File is empty";
+
+    fseek(fp, 0, SEEK_SET);
+    Rpp8u *binaryContent = (Rpp8u *)malloc(fsize);


We can try doing a static allocation as abishek said, but read the data
which is of size fsize only

Instead of dynamically allocating for every variant, better to static allocate as abishek said

sampath1117 · 2024-01-25T07:46:29Z

utilities/test_suite/rpp_test_suite_voxel.h

+        int depthStride = descriptorPtr3D->strides[2];
+        int rowStride = descriptorPtr3D->strides[3];
+        int channelStride = descriptorPtr3D->strides[1];
+        int matchedIdx = 0;

        for(int c = 0; c < descriptorPtr3D->dims[1]; c++)


Same comment as before
Need not require dims[1] loop for PLN1

sampath1117 · 2024-01-25T13:22:12Z

@sampath1117 Please do a round of review and ensure tests passing

@r-abishek I have tested the code from my end
All tests pass on HOST and HIP for both image and voxel functions

…#222) Bumps [rocm-docs-core](https://github.com/RadeonOpenCompute/rocm-docs-core) from 0.24.1 to 0.24.2. - [Release notes](https://github.com/RadeonOpenCompute/rocm-docs-core/releases) - [Changelog](https://github.com/RadeonOpenCompute/rocm-docs-core/blob/develop/CHANGELOG.md) - [Commits](ROCm/rocm-docs-core@v0.24.1...v0.24.2) --- updated-dependencies: - dependency-name: rocm-docs-core dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump rocm-docs-core from 0.24.0 to 0.24.1 in /docs/.sphinx (r-abishek#219) Bumps [rocm-docs-core](https://github.com/RadeonOpenCompute/rocm-docs-core) from 0.24.0 to 0.24.1. - [Release notes](https://github.com/RadeonOpenCompute/rocm-docs-core/releases) - [Changelog](https://github.com/RadeonOpenCompute/rocm-docs-core/blob/develop/CHANGELOG.md) - [Commits](ROCm/rocm-docs-core@v0.24.0...v0.24.1) --- updated-dependencies: - dependency-name: rocm-docs-core dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump rocm-docs-core from 0.24.1 to 0.24.2 in /docs/.sphinx (r-abishek#222) Bumps [rocm-docs-core](https://github.com/RadeonOpenCompute/rocm-docs-core) from 0.24.1 to 0.24.2. - [Release notes](https://github.com/RadeonOpenCompute/rocm-docs-core/releases) - [Changelog](https://github.com/RadeonOpenCompute/rocm-docs-core/blob/develop/CHANGELOG.md) - [Commits](ROCm/rocm-docs-core@v0.24.1...v0.24.2) --- updated-dependencies: - dependency-name: rocm-docs-core dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump rocm-docs-core from 0.24.2 to 0.25.0 in /docs/.sphinx (r-abishek#225) Bumps [rocm-docs-core](https://github.com/RadeonOpenCompute/rocm-docs-core) from 0.24.2 to 0.25.0. - [Release notes](https://github.com/RadeonOpenCompute/rocm-docs-core/releases) - [Changelog](https://github.com/RadeonOpenCompute/rocm-docs-core/blob/develop/CHANGELOG.md) - [Commits](ROCm/rocm-docs-core@v0.24.2...v0.25.0) --- updated-dependencies: - dependency-name: rocm-docs-core dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump rocm-docs-core from 0.25.0 to 0.26.0 in /docs/.sphinx (r-abishek#228) Bumps [rocm-docs-core](https://github.com/RadeonOpenCompute/rocm-docs-core) from 0.25.0 to 0.26.0. - [Release notes](https://github.com/RadeonOpenCompute/rocm-docs-core/releases) - [Changelog](https://github.com/RadeonOpenCompute/rocm-docs-core/blob/develop/CHANGELOG.md) - [Commits](ROCm/rocm-docs-core@v0.25.0...v0.26.0) --- updated-dependencies: - dependency-name: rocm-docs-core dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * ENABLE ASAN packaging RPP (r-abishek#227) * ENABLE ASAN packaging RPP * Add LICENSE File to RPP Package * Update README.md Pre-reqs * RPP Doxygen Update 2 (r-abishek#226) * Remove rpp core * Doxygen fixes for rpp.h * Update enum values per line to 1 * Doxygen fixes for rppdefs.h * Doxygen fixes for rppi.h * Doxygen fixes for rppt.h * Doxygen fixes for rppversion.h * Rename Filter struct to GenericFilter to avoid Doxygen autoreference * Add deprecated docs for image ops * Doxygen fixes for all rppi headers * Doxygen fixes for all rppt headers, and replace \ingroup with \addtogroup * Change deprecation note - "To be deprecated" * Add \deprecated flag * Minor fix * Add output samples for doxygen * Add doxygen input * Fix docs for brightness * Add correct image path to Doxyfile * Add input/output images for brightness * Fix doxygen for brightness, blend and gamma correction * Change to add "Sample" to docs * Fix doxygen for color_cast, color_jitter, color_twist * Fix all doxygen docs for color augmentations * Fix doxygen for filter funcs * doxygen fixes for data exchange ops * Add sample second input to docs * Add more sample ouputs for doxygen docs * Fix doxygen docs for gridmask and spatter * Fix docs for swap_channels * Fix doxygen docs for snp, shot and gaussian noise * Fix doxygen docs for blend and non linear blend * Fix doxygen docs for statistical ops * remove boost library * cmake fix * code cleanup * suppress nodiscard attribute error * filesystem modification * filesystem modification * code cleanup * data type fix --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: arvindcheru <90783369+arvindcheru@users.noreply.github.com> Co-authored-by: Kiriti Gowda <kiritigowda@gmail.com> Co-authored-by: Abishek <52214183+r-abishek@users.noreply.github.com>

HazarathKumarM added 2 commits January 24, 2024 14:38

change golden outputs from .csv files to .bin files

ae84527

Changed comparision funtions to use .bin files

570b41a

r-abishek requested changes Jan 24, 2024

View reviewed changes

r-abishek assigned HazarathKumarM Jan 24, 2024

r-abishek added the enhancement New feature or request label Jan 24, 2024

HazarathKumarM added 2 commits January 25, 2024 01:53

Address review comments

7b08971

minor change

7ea3ac6

sampath1117 reviewed Jan 25, 2024

View reviewed changes

HazarathKumarM added 2 commits January 25, 2024 05:15

Address review comments

add4783

minor change

9bf5305

r-abishek approved these changes Jan 25, 2024

View reviewed changes

r-abishek changed the base branch from master to ar/test_suite_upgrade_4_csv_bin January 25, 2024 19:01

r-abishek merged commit c2b7182 into r-abishek:ar/test_suite_upgrade_4_csv_bin Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPP Test Suite Upgrade - CSV to Binary Conversion #222

RPP Test Suite Upgrade - CSV to Binary Conversion #222

HazarathKumarM commented Jan 24, 2024

r-abishek left a comment

r-abishek Jan 24, 2024

HazarathKumarM Jan 25, 2024

r-abishek Jan 24, 2024

HazarathKumarM Jan 25, 2024

sampath1117 Jan 25, 2024

r-abishek Jan 24, 2024

HazarathKumarM Jan 25, 2024

r-abishek Jan 24, 2024

HazarathKumarM Jan 25, 2024

r-abishek Jan 24, 2024

HazarathKumarM Jan 25, 2024

r-abishek Jan 24, 2024

HazarathKumarM Jan 25, 2024

r-abishek commented Jan 24, 2024

sampath1117 left a comment

sampath1117 Jan 25, 2024

HazarathKumarM Jan 25, 2024

sampath1117 Jan 25, 2024

HazarathKumarM Jan 25, 2024

sampath1117 Jan 25, 2024

HazarathKumarM Jan 25, 2024

sampath1117 Jan 25, 2024

HazarathKumarM Jan 25, 2024

sampath1117 Jan 25, 2024

HazarathKumarM Jan 25, 2024

sampath1117 Jan 25, 2024

HazarathKumarM Jan 25, 2024

sampath1117 Jan 25, 2024

HazarathKumarM Jan 25, 2024

sampath1117 Jan 25, 2024

HazarathKumarM Jan 25, 2024

sampath1117 commented Jan 25, 2024

RPP Test Suite Upgrade - CSV to Binary Conversion #222

RPP Test Suite Upgrade - CSV to Binary Conversion #222

Conversation

HazarathKumarM commented Jan 24, 2024

r-abishek left a 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

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

r-abishek commented Jan 24, 2024

sampath1117 left a 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

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

sampath1117 commented Jan 25, 2024