Modified backend wrap() to accept output arg #2328

mlloreda · 2018-10-25T20:36:50Z

No description provided.

umar456 · 2018-10-26T00:47:34Z

Please add tests.

mark-poscablo · 2018-10-26T22:38:16Z

src/backend/cpu/wrap.cpp

    in.eval();
+    const dim4 idims = in.dims();
+    const dim4 odims(ox, oy, idims[2], idims[3]);
+    out = createValueArray<T>(odims, scalar<T>(0));


I think you meant to omit these lines, correct? Since this will make wrap write its results to the memory returned by createValueArray and not the already provided space in the out argument. I see that you omitted this in the CUDA and OpenCL backends so I guess it was just an accident that you left this here.

umar456 · 2019-08-08T17:24:11Z

src/api/c/wrap.cpp

+                  const dim_t sx, const dim_t sy,
+                  const dim_t px, const dim_t py,
+                  const bool is_column) {
+    if (out == 0) return AF_ERR_ARG;


I think you should do the same conversion you did in the approx_v2 functions here for the error messages.

umar456 · 2019-08-08T17:25:44Z

src/api/c/wrap.cpp


-        dim_t nx = (ox + 2 * px - wx) / sx + 1;
-        dim_t ny = (oy + 2 * py - wy) / sy + 1;
+        ARG_ASSERT(4, wx > 0);


These indicies will be incorrect in the older versions of approx. These checks should really be in the API level functions. Unfortunately it will be error prone to duplicate this everywhere. I am not sure how to fix this cleanly.

umar456 · 2019-08-08T17:27:56Z

test/wrap.cpp

+              AF_ERR_ARG);
+}
+
+class ArgDim {


How is this different from dim4?

umar456 · 2019-08-08T17:28:49Z

test/wrap.cpp

+   public:
+    WindowDims *wc_;
+    StrideDims *sc_;
+    PadDims *pc_;


Do these types really need to be pointers?

umar456 · 2019-08-08T17:34:04Z

src/api/c/wrap.cpp

-        af_array output;
+        if (allocate_out) { *out = createHandle(out_dims, in_type); }
+
+        DIM_ASSERT(1, getInfo(*out).dims() == out_dims);


Suggested change

DIM_ASSERT(1, getInfo(*out).dims() == out_dims);

// The out pointer can be passed in to the function by the user

DIM_ASSERT(1, getInfo(*out).dims() == out_dims);

umar456 · 2019-08-08T17:41:46Z

src/backend/cpu/kernel/wrap.hpp

@@ -25,6 +29,8 @@ void wrap_dim(Param<T> out, CParam<T> in, const dim_t wx, const dim_t wy,
    af::dim4 istrides = in.strides();
    af::dim4 ostrides = out.strides();

+    std::fill(begin(out), end(out), scalar<T>(0));


I don't like this. the out array should be zeroed before we get here. Instead of createHandle, you should use createValueArray.

If the out array is passed into the function, we can stipulate that the values will be summed into the output array. If they want the originals they need to pass a constant array or zero beforehand.

Minor cleanup.

mark-poscablo reviewed Oct 26, 2018

View reviewed changes

mlloreda force-pushed the out-wrap branch 10 times, most recently from 1c43ebd to a7a179c Compare November 1, 2018 20:44

umar456 force-pushed the out-wrap branch from a7a179c to 26ff815 Compare November 1, 2018 21:15

mlloreda force-pushed the out-wrap branch 2 times, most recently from 21583a5 to 559d59f Compare November 2, 2018 15:26

mark-poscablo force-pushed the out-wrap branch 2 times, most recently from 1d28ee5 to 0b88f98 Compare July 29, 2019 19:24

mark-poscablo force-pushed the out-wrap branch from 0b88f98 to 299b710 Compare August 8, 2019 17:03

umar456 requested changes Aug 8, 2019

View reviewed changes

mark-poscablo force-pushed the out-wrap branch from 299b710 to ac7d508 Compare August 9, 2019 00:25

mark-poscablo force-pushed the out-wrap branch from 24fc926 to 6833a3a Compare August 24, 2019 15:22

umar456 previously approved these changes Aug 29, 2019

View reviewed changes

umar456 dismissed their stale review via 9581ce8 August 29, 2019 23:41

mlloreda and others added 7 commits February 7, 2020 16:06

Modified backend wrap() to accept output arg

b2ac024

[WIP] Testing wrap.

d1dd7cf

Eval output and fill it with zeros before operation

21c49a0

Used test fixture to improve wrap tests.

274a376

WIP: attempt at value parameterization

6b6e5f4

Updated fill iterator. Removed ArrayIterator

1168d43

Testing wrap API via value parameterization.

737e14e

mlloreda and others added 12 commits February 7, 2020 16:06

Cleaned up supporting code for parameterization

98f4c8a

Made getInput/Gold() static function.

932b47b

Minor cleanup.

Added header to args table.

b6ec840

Defined GTEST_LINKED_AS_SHARED_LIBRARY

a38cbe3

Remove leftover getWritableArray calls and getHandle modification

2c39429

Move logic for handling output array allocation to af_wrap

030741e

Assert out_dims is correct whether out was created or already exists

ca0e1bc

Add tests for af_wrap_v2 and include declaration in image.h

346a503

Clang-format files while keeping specially formatted sections

222f85c

Revert src/backend/cpu/Param.hpp to arrayfire master (a362e49)

1cfa82c

Add back line after copyright notice in test/wrap.cpp

192024e

Add offset to output pointer in wrap OpenCL kernel

f3e54f1

mark-poscablo force-pushed the out-wrap branch from 9581ce8 to bf4c9e7 Compare February 7, 2020 22:20

Mark Poscablo added 8 commits February 7, 2020 16:33

Add af_wrap_v2 to unified backend API

8bba30c

Convert tests into test fixtures, add tests for null arguments

7a31b47

Integrate new test fixtures with existing ones

8fcdfc4

Move try-catch from af_wrap_common to C API functions

a41683d

Remove pointers and change small classes into structs in test

42698cc

Remove std fill in CPU kernel, add value arg in genTestOutputArray

3e1b6d3

Call basic TestOutputArrayInfo::init inside subarray init

f420606

Change return type of af_wrap_common to void

7d847a4

mark-poscablo force-pushed the out-wrap branch from bf4c9e7 to 7d847a4 Compare February 7, 2020 22:35

umar456 approved these changes Feb 8, 2020

View reviewed changes

umar456 merged commit a049d75 into arrayfire:master Feb 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Modified backend wrap() to accept output arg #2328

Modified backend wrap() to accept output arg #2328

Uh oh!

mlloreda commented Oct 25, 2018

Uh oh!

umar456 commented Oct 26, 2018

Uh oh!

mark-poscablo Oct 26, 2018

Uh oh!

umar456 Aug 8, 2019

Uh oh!

umar456 Aug 8, 2019

Uh oh!

umar456 Aug 8, 2019

Uh oh!

umar456 Aug 8, 2019

Uh oh!

umar456 Aug 8, 2019

Uh oh!

umar456 Aug 8, 2019

Uh oh!

Uh oh!

	DIM_ASSERT(1, getInfo(*out).dims() == out_dims);
	// The out pointer can be passed in to the function by the user
	DIM_ASSERT(1, getInfo(*out).dims() == out_dims);

Modified backend wrap() to accept output arg #2328

Modified backend wrap() to accept output arg #2328

Uh oh!

Conversation

mlloreda commented Oct 25, 2018

Uh oh!

umar456 commented Oct 26, 2018

Uh oh!

mark-poscablo Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

umar456 Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

umar456 Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

umar456 Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

umar456 Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

umar456 Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

umar456 Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!