Implement approx1 and approx2 along specified dimensions. #2297

mlloreda · 2018-09-19T14:21:55Z

New approx1/2 functions which allow the user to select which dimension(s) to interpolate across.

Documentation has been updated and new tests have been added to @pavanky's original #1990 PR.

include/af/signal.h

syurkevi · 2018-09-19T21:59:10Z

include/af/signal.h

-   \param[in]  offGrid is the value that will set in the output array when certain index is out of bounds
-   \return     the array with interpolated values
+   \param[in]  yi is the input array
+   \param[in]  xo array contains the interpolation locations


Not liking the x,y parameters. what does xo stand for? in and pos seem to make more sense.

9prady9 · 2018-10-06T02:49:50Z

docs/details/signal.dox

@@ -211,54 +211,109 @@ respectively, then the possible batch operations are as follows.
 \defgroup signal_func_approx1 approx1
 \ingroup approx_mat

-approx1 interpolates data along a single dimension.
+approx1 interpolates data along one dimension. Interpolation computes
+for values of unknown points out of known ones.


Looks like a typo in second sentence. Just computes instead of computes for is sufficient.

Also, lets keep the first sentence short as that appears in the brief descriptions of the documentation. So, move this line into a separate line so that approx interpolates data along one dimension comes in brief description.

I agree with Pradeep on moving the second sentence to the next paragraph. That way too you can expand more on what interpolation means and not be restricted to keeping it short because it's part of the brief description. I mean, for me at least, the way you described interpolation works for extrapolation as well, ~~so I think it needs a little more detail and so I think it should be moved after the first line/brief description~~ I saw that you do describe interpolation in more detail below.

mark-poscablo · 2018-10-11T16:25:56Z

Maybe it would be helpful too if I review the docs as well, reading it from a newcomer's perspective.

mark-poscablo · 2018-10-11T17:00:34Z

docs/details/signal.dox

+the `grid_beg` and `grid_step` arguments.
+
+The position array (<=1D), `pos`, contains a combination of known and
+unkown positions/indices on the interpolation grid. The position


Typo: "unkown". Also you already wrote "The position array, pos" above. Also why <=1D? Is it because pos can be just a single index as well?

mark-poscablo · 2018-10-11T17:19:40Z

docs/details/signal.dox

+dimension in which the measurements will be made in the input array.
+
+Any unknown position/index values that lie within the interpolation
+grid range will be calculated for using interpolation. Unknown points


What if we replace "calculated for" with "determined" (and "computes for" with "determines" in the very first line)?

mark-poscablo · 2018-10-11T17:22:46Z

docs/details/signal.dox

+the `off_grid` value. Known points will be replaced with corresponding
+values from the input array.
+
+The interpolation dimension, `interp_dim`, is the leading dimension


I somehow feel like this description should be closer to the description of the pos argument, since they both say something about the leading dimension. But I get it that the pos array must be intended to be along the interp_dim dimension, correct?

mark-poscablo · 2018-10-11T17:51:43Z

docs/details/signal.dox

+
+The multi-dimensional (>=1D) input array, `in`, contains known values
+to be used for interpolation. These known values lie on a 1D
+interpolation grid - a one-dimensional, uniformly-spaced grid of


I'm kind of getting confused on "1D interpolation grid". Did you mean "one dimensional" to describe the interpolation, and not the grid (the input array)?

mark-poscablo · 2018-10-11T18:03:11Z

docs/details/signal.dox

+equally spaced indices in the range of [0,n) can be achieved with grid
+begin and step values of 0 and 1, respectively. A standard uniform
+grid will be used in the case that the interpolating dimension as well
+as the grid start and step values are not passed in.


So I'm still not sure what grid_beg and grid_step are for, given that we have pos already. Is grid_beg where you start basing the interpolation on? Say I have a 1D input array with 100 elements that look like a piecewise function, where the first half [0, 50) looks like a quadratic function and the second half [50, 100) looks like a line, and I set pos to be indices within the second half, and I don't want the first half to affect the interpolation - do I set grid_beg then to be 50?

mark-poscablo · 2018-10-11T18:09:52Z

There, I just added comments on the approx1 docs, at least. ~~I think they're not showing up here in this page though since I chose "Add single comment", sorry, but please check out the "Files changed" tab~~ They show up now, sorry, just had to refresh the page. In any case I appreciate adding more detail on the approx functions! I think the the current ones are too short to be really helpful.

9prady9 · 2018-10-15T14:50:04Z

docs/details/signal.dox

-locations.
+The input array, `in`, is comprised of values which lie on a range of
+indices defined by `idx_start` and `idx_step` [idx_start:idx_step:N]
+along dimension `interp_dim`.


Like you did below for off_grid, we should also explain that the API of approx1 that doesn't take idx_start and idx_step assumes them to be of so and so values.
Similar, explanation for interp_dim is also needed.

9prady9 · 2018-10-15T14:53:33Z

docs/details/signal.dox

-in the range [0, n) along each dimension.
-The positions are sampled with respect to data at these locations.
+approx2 interpolates data along two dimensions. Interpolation computes
+for values of unknown points out of known ones.


The second sentence is not needed, this makes the short description too long to appear in UI - check the very first sentence in this page. Also, you talk about what interpolation does below also. So, this is not needed here in my opinion.

9prady9 · 2018-10-15T14:59:06Z

docs/details/signal.dox

+
+Interpolation positions outside of the interpolation grid range are
+not extrapolated. Instead, those values default to `off_grid`, whose
+default value is 0.


Looks like last two paragraphs are giving out only redundant info that is already explained in earlier paragraphs.

mlloreda · 2018-10-16T03:52:15Z

mlloreda · 2018-10-16T03:52:22Z

mlloreda · 2018-10-16T03:59:59Z

See arrayfire/assets#7 for assets PR.

Update: assets is merged now.

9prady9 · 2018-10-16T05:05:26Z

@mlloreda I think it looks great and definitely much better what we had earlier.

The docs job failed though. Looks like assets submodule isn't not updated to your new changes.

Also updated API version guard.

Step size of zero generates an invalid sampling grid.

Also: - Removed unnecessary header includes from cpu/approx.cpp - Minor c/approx.cpp cleanup - Minor approx1/2 test cleanup

- Check for non-monotonic positions. - Check for approx behavior with af::Inf values.

Also referencing snippets from test framework.

The approx CPU kernel implementation was calling the interp() kernel with the incorrect dimension values. This was causing the CPPUniform{Row,Column}MajorInterpolation tests to fail previously. Also improved approx1 and approx2 tests.

All tests performing interpolation. Improved test names, removed unnecessary tests, reordered tests

syurkevi reviewed Oct 1, 2018

View reviewed changes

mlloreda force-pushed the approx branch 3 times, most recently from ca167f2 to 2134d4d Compare October 4, 2018 14:25

9prady9 reviewed Oct 6, 2018

View reviewed changes

mark-poscablo reviewed Oct 11, 2018

View reviewed changes

9prady9 reviewed Oct 15, 2018

View reviewed changes

mlloreda force-pushed the approx branch from 94f290f to 2ad1cd4 Compare October 16, 2018 03:54

mlloreda force-pushed the approx branch 6 times, most recently from 9d93431 to 594de3a Compare October 16, 2018 14:15

pavanky added 4 commits October 16, 2018 09:17

skeleton for approx along any dimension

b96df6f

CPU: Adding the approx1 and approx2 along any given dimension.

eadd090

CUDA: Adding the approx1 and approx2 along any given dimension.

2bb7f72

OpenCL: Adding the approx1 and approx2 along any given dimension.

377fcdb

pavanky and others added 17 commits October 16, 2018 09:17

PERF, OpenCL: Fixes the launch config for approx1

937a40a

TEST: Adding tests for approx1 and approx2 along other dimensions.

e41c07c

Added missing \params to new uniform approx2().

d8b05fd

Also updated API version guard.

Throw exception for invalid step size of 0.

2c7155d

Step size of zero generates an invalid sampling grid.

Tests for approx1/2() to verify API functionality.

c05bea0

Also: - Removed unnecessary header includes from cpu/approx.cpp - Minor c/approx.cpp cleanup - Minor approx1/2 test cleanup

Improved documentation for approx1/2().

6569aa1

Included uniform approx usage snippets in docs.

8ba81e7

New approx1 tests:

6bf4c65

- Check for non-monotonic positions. - Check for approx behavior with af::Inf values.

More approx1/2 documentation improvements.

20cca41

Also referencing snippets from test framework.

New approx1/2 tests. Also improved tests and docs.

bcf540c

Improved naming convention for client API.

6e3532d

Improved documentation.

b190fe1

Fixed approx2() CPU kernel issue. Improved tests.

991c7e2

The approx CPU kernel implementation was calling the interp() kernel with the incorrect dimension values. This was causing the CPPUniform{Row,Column}MajorInterpolation tests to fail previously. Also improved approx1 and approx2 tests.

Improved approx1 tests as well as docs sample.

479f5df

All tests performing interpolation. Improved test names, removed unnecessary tests, reordered tests

Refactored approx2 tests and improved doc sample.

2e446cc

Improved approx2() API.

f2fab41

Rewrote approx1/2 documentation.

501506d

mlloreda force-pushed the approx branch from 594de3a to f8abeae Compare October 16, 2018 14:18

Improved approx1/2 documentation.

04c2d5e

mlloreda force-pushed the approx branch from f8abeae to 04c2d5e Compare October 16, 2018 14:19

9prady9 added the in-progress label Oct 17, 2018

Restructure approx documentation.

ebb5e2a

9prady9 approved these changes Oct 20, 2018

View reviewed changes

9prady9 removed the in-progress label Oct 20, 2018

9prady9 merged commit b136f20 into arrayfire:master Oct 20, 2018

mark-poscablo mentioned this pull request Oct 29, 2018

Implement approx1 and approx2 along a given dimension. #1955

Closed

greennbeans mentioned this pull request Jul 17, 2020

[Perf] Slow approx1 performance in v3.7.x #2975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement approx1 and approx2 along specified dimensions. #2297

Implement approx1 and approx2 along specified dimensions. #2297

mlloreda commented Sep 19, 2018

syurkevi Sep 19, 2018

mlloreda Oct 1, 2018

9prady9 Oct 6, 2018

mark-poscablo Oct 11, 2018 •

edited

Loading

mark-poscablo commented Oct 11, 2018

mark-poscablo Oct 11, 2018 •

edited

Loading

mark-poscablo Oct 11, 2018

mark-poscablo Oct 11, 2018

mark-poscablo Oct 11, 2018

mark-poscablo Oct 11, 2018 •

edited

Loading

mark-poscablo commented Oct 11, 2018 •

edited

Loading

9prady9 Oct 15, 2018

9prady9 Oct 15, 2018

9prady9 Oct 15, 2018

mlloreda commented Oct 16, 2018

mlloreda commented Oct 16, 2018

mlloreda commented Oct 16, 2018 •

edited by 9prady9

Loading

9prady9 commented Oct 16, 2018

Implement approx1 and approx2 along specified dimensions. #2297

Implement approx1 and approx2 along specified dimensions. #2297

Conversation

mlloreda commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-poscablo Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

mark-poscablo commented Oct 11, 2018

mark-poscablo Oct 11, 2018 • edited Loading

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

mark-poscablo Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

mark-poscablo commented Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlloreda commented Oct 16, 2018

mlloreda commented Oct 16, 2018

mlloreda commented Oct 16, 2018 • edited by 9prady9 Loading

9prady9 commented Oct 16, 2018

mark-poscablo Oct 11, 2018 •

edited

Loading

mark-poscablo Oct 11, 2018 •

edited

Loading

mark-poscablo Oct 11, 2018 •

edited

Loading

mark-poscablo commented Oct 11, 2018 •

edited

Loading

mlloreda commented Oct 16, 2018 •

edited by 9prady9

Loading