Skip to content

Add measured locations as parameters for approx1 and approx2 #160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 29, 2017
Merged

Add measured locations as parameters for approx1 and approx2 #160

merged 5 commits into from
Sep 29, 2017

Conversation

shyams2
Copy link
Contributor

@shyams2 shyams2 commented Sep 26, 2017

I've implemented an interp1d and interp2d function similar to the one found in scipy. Rather than having the user manually transform the input points from [0, N-1] when using approx1 and approx2, this function takes care of the transformation by taking the input data points in addition to the interpolation points.

However, there is one issue: I'm using sum to get a scalar value:

dx   = sum(x_input[1, 0, 0, 0] - x_input[0, 0, 0, 0])
pos0 = (x_interpolated - sum(x_input[0, 0, 0, 0]))/dx

Do you recommend using something else?

@pavanky
Copy link
Member

pavanky commented Sep 26, 2017

@ShyamSS-95 May be we can expand approx1 / approx2 by adding additional variables instead of introducing new functions ?

You could also convert the scaling into a function

@af.broadcast
def _scale_pos(x_orig, x_curr):
    x0 = x_orig[0, 0, 0, 0]
    dx   = af.abs(x_orig[1, 0, 0, 0] - x0)
    return (x_curr - x0) / dx

Since you are using broadcasting, you dont need to get the scalar outside.

@shyams2
Copy link
Contributor Author

shyams2 commented Sep 27, 2017

@pavanky I've made the changes to approx1/2 itself. Take a look and let me know if its fine :)

@pavanky
Copy link
Member

pavanky commented Sep 27, 2017

@ShyamSS-95

  1. I am a bit hesitant to change the variable names, but the original names were terrible so let me see if I can find out what other libs are calling them.

  2. This breaks the behavior of old approx. I was thinking the new parameter will be optional.

@shyams2
Copy link
Contributor Author

shyams2 commented Sep 27, 2017

@pavanky

Ah OK. But I'm still not clear on how that would work. To clarify, would it be something like this?:

approx1(signal, pos0 = None, method=INTERP.LINEAR, off_grid=0.0,
        x_interpolated = None, x_input = None
       ):
   output = Array() 
   if(x_interpolated is not None and x_input is not None):
        pos0   = _scale_pos_axis0(x_interpolated, x_input)

    safe_call(backend.get().af_approx1(c_pointer(output.arr), signal.arr, pos0.arr,
                                       method.value, c_float_t(off_grid)))
   return output

Then for the old behaviour, this can be used:
approx1(signal, pos0)
For the interp1d like behaviour:
approx1(signal, x_interpolated = whatever, x_input = whatever)

@pavanky
Copy link
Member

pavanky commented Sep 27, 2017

@ShyamSS-95 x_interpolated and pos0 should be the same thing?

@shyams2
Copy link
Contributor Author

shyams2 commented Sep 27, 2017

@pavanky

Oh. So, if x_interpolated alone is provided, then it is assumed to have pos0 form(when x_input is None)?

@pavanky
Copy link
Member

pavanky commented Sep 27, 2017

@ShyamSS-95 yes

@shyams2
Copy link
Contributor Author

shyams2 commented Sep 28, 2017

@pavanky
I've made changes to retain the earlier approx behaviour as well. Let me know if it's alright, and what the variable names need to be changed to.

@pavanky
Copy link
Member

pavanky commented Sep 28, 2017

One last thing, can you shorten the input names ? May be something like numpy? https://docs.scipy.org/doc/numpy/reference/generated/numpy.interp.html

@pavanky pavanky changed the title Added interp1d and interp2d Add option to specify the measured locations for approx1 and approx2 Sep 28, 2017
@pavanky pavanky changed the title Add option to specify the measured locations for approx1 and approx2 Add measured locations as parameters for approx1 and approx2 Sep 28, 2017
@pavanky pavanky merged commit 718b8fc into arrayfire:master Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants