Skip to content
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

Multimethods for mathematical functions #64

Merged
merged 9 commits into from
Jul 23, 2020

Conversation

joaosferreira
Copy link
Collaborator

This pull request further adds multimethods that target mathematical functions. I'll name the multimethods added according to the NumPy docs sectioning:

Trigonometric functions

  • degrees
  • radians
  • unwrap

Rounding

  • around (and its aliase round_)
  • fix

Sums, products, differences

  • cumprod
  • cumsum
  • nancumprod
  • nancumsum
  • ediff1d
  • cross
  • trapz

Other special functions

  • i0
  • sinc

Arithmetic operations

  • float_power

Handling complex numbers

  • angle
  • real
  • imag
  • conjugate

Miscellaneous

  • convolve
  • clip
  • nan_to_num
  • real_if_close
  • interp

Some thoughts about things to address moving forward with the PR:

  1. There are no default implementations in this initial commit. I intend to fix the failing tests first before I start pushing some. The failing tests are all in the XND backend and are almost all the same.
  2. I noticed that the multimethod for diff has a different signature from the NumPy's function (the latter has more arguments). I'm wondering if these were added after the unumpy's multimethod was written.
  3. I think some older multimethods for ufuncs don't have tests so I might add those as well.

diffs = ary[1:] - ary[:-1]

if to_end is None:
to_end = asarray([], dtype=ary.dtype)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here? Concatenating an empty array does nothing.

Copy link
Collaborator Author

@joaosferreira joaosferreira Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I could do the other way around like:

if to_end is not None:
    diffs = concatenate([diffs, ravel(to_end)])

Seems better this way!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append to a list, check for trivial case and return early.

@joaosferreira
Copy link
Collaborator Author

There's a type error arising from the call to where in nancumprod and nancumsum defaults. Should I ignore them?

@hameerabbasi
Copy link
Collaborator

Yes, just add type: ignore

@joaosferreira
Copy link
Collaborator Author

The default for interp was based on NumPy's implementation and uses the equation of slopes to compute the y-coordinates element-wise.

I'm still not sure how to add the left and right cases. Also, it needs to take into consideration complex numbers.

@peterbell10
Copy link
Contributor

Instead of clipping x, couldn't you do something like this:

# Where we return currently
result = (y0 * x1_dist + y1 * x0_dist) / x_dist 

left = fp[0] if left is None else left
right = fp[-1] if right is None else right
result = where(x < xp[0], left, result)
return where(x > xp[-1], right, result)

You will need to fixup idxs as well to make sure there are no out of bounds accesses when interpolating.

@joaosferreira
Copy link
Collaborator Author

I've pushed the changes to _interp_default that add the left and right cases. It also works for complex numbers.

@joaosferreira
Copy link
Collaborator Author

NumPy's implementation for unwrap uses item assignment which is not supported in backends with read-only arrays. Maybe there's a way to implement the default without using item assignment?

@joaosferreira joaosferreira marked this pull request as ready for review July 20, 2020 17:09
@hameerabbasi
Copy link
Collaborator

I'd recommend you first do the calculation as if slice1 was only used for reading, not assignment, then, when it's being used for assignment: https://github.com/numpy/numpy/blob/v1.19.0/numpy/lib/function_base.py#L1540

I'd recommend you replace that line with a concatenate in the appropriate axis with a sub-array in the appropriate axis. A 1D version is below, I'll leave it as an exercise to come up with N-D (hint: tuple with slice(None) and 0, and axis in concatenate).

up_slice1 = p[slice1] + ph_correct.cumsum(axis)
up = concatenate([p[0], up_slice1])

dd = diff(p, axis=axis)

slice0 = [slice(None)] * nd
slice0[axis] = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use slice(0, 1) to grab the 0 value without also collapsing the dimension. That will avoid the need to use expand_dims later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, that way we avoid an unnecessary multimethod call.

round_ = around


def _fix_default(x, out=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the same as trunc.

The truncated value of the scalar x is the nearest integer i which is closer to zero than x is.

Copy link
Collaborator Author

@joaosferreira joaosferreira Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They seem to be the same, yes. Refactor the default in terms of trunc then? Something like:

def _fix_default(x, out=None):
    x = trunc(x)

    if out is None:
        return x
    else:
        copyto(out, x)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return trunc(x, out=out)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right! Forgot about the out kwarg in ufuncs. 😅

@hameerabbasi
Copy link
Collaborator

We've merged the fix we talked about in the meeting in Quansight-Labs/uarray#246. Pushing an empty commit with git commit --allow-empty (or any changes) should fix CI if there are no other breaking changes.

@joaosferreira
Copy link
Collaborator Author

I think this can be merged now.

@hameerabbasi hameerabbasi merged commit 6edccfd into Quansight-Labs:master Jul 23, 2020
@hameerabbasi
Copy link
Collaborator

Thanks, @joaosferreira!

@joaosferreira joaosferreira deleted the math-functions branch July 23, 2020 21:52
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.

3 participants