Skip to content

Add specifications for array manipulation functions #42

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 14 commits into from
Sep 28, 2020

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Sep 14, 2020

This PR

  • adds specifications for array manipulation functions.
  • is derived from comparing API signatures across array libraries.

Notes

  • This list of array manipulation functions is an initial set of array manipulation functions which can pave the way for additional specs in subsequent pull requests. These functions were identified as the set of functions with the broadest support among array libraries and relatively higher usage among downstream libraries.

  • Some comments/questions regarding particular APIs...

    • concat: CuPy requires a tuple rather than a sequence for first argument. Went with tuple as more consistent with rest of specification (e.g., we require a list of axes to be specified as a tuple, but not a sequence). What happens if provided arrays having different dtypes? What should we the dtype of the returned array? How do type promotion rules factor in here?

      • Answer: regular type promotion rules. Between data type families, behavior is left unspecified.
    • expand_dims: NumPy supports providing a tuple or an int. All other array libraries considered support only an int. Torch names this method unsqueeze. Went with expand_dims and only accepting an int for the second positional argument.

    • flip: TensorFlow lacks this exact API. Torch/CuPy/ spec axis/dims as position argument. Based proposal on NumPy where axis is a keyword argument, as more versatile.

    • reshape: Torch requires a tuple (does not allow int). TensorFlow requires shape to be a int32/int64 tensor. NumPy allows providing an int as shorthand. Based proposal on Torch's more restricted API for consistency.

    • roll: TensorFlow requires tensors for axis and shifts.

    • squeeze: Torch only allows specifying one axis and does not error if you attempt to squeeze non-singleton dimensions. NumPy/TensorFlow error if you attempt to squeeze a dimension which is not 1. Sided with Torch regarding error behavior, as not clear why attempting to squeeze a non-singleton dimension should error.

    • stack: CuPy requires a tuple rather than a sequence for the first argument. Went with tuple for same reasons as in concat. Same dtype question(s) apply as for concat above.

@rgommers
Copy link
Member

rgommers commented Sep 14, 2020

  • concat: CuPy requires a tuple rather than a sequence for first argument. Went with tuple as more consistent with rest of specification (e.g., we require a list of axes to be specified as a tuple, but not a sequence). What happens if provided arrays having different dtypes? What should we the dtype of the returned array? How do type promotion rules factor in here?

I'd say apply the regular type casting rules. So regular within-dtype-family upcasting (intxx, floatxx) is undefined. Quickly checked numpy and pytorch, that's what they both seem to do.

  • flip: TensorFlow lacks this exact API. Torch/CuPy/ spec axis/dims as position argument. Based proposal on NumPy where axis is a keyword argument, as more versatile.

This makes sense to include (but leaving out fliplr and flipud). The indexing-based equivalent ([...,::=1,...]) is too weird.

  • reshape: Torch requires a tuple (does not allow int). TensorFlow requires shape to be a int32/int64 tensor. Based proposal on NumPy as providing an int is convenient shorthand.

I'd be okay with just a tuple for consistency. It's pretty unusual to do reshape with an integer (or a 1-D array in general), and some_1d_array.shape will give a length-1 tuple.

  • roll: TensorFlow requires tensors for axis and shifts.

That's a little odd, regular ints/tuples should be fine I'd think.

  • squeeze: Torch only allows specifying one axis and does not error if you attempt to squeeze non-singleton dimensions. NumPy/TensorFlow error if you attempt to squeeze a dimension which is not 1. Sided with Torch regarding error behavior, as not clear why attempting to squeeze a non-singleton dimension should error.

+1

  • stack: CuPy requires a tuple rather than a sequence for the first argument. Went with tuple for same reasons as in concat. Same dtype question(s) apply as for concat above.

Sounds good to me.

Functions I noticed that you left out are:

  • moveaxis, swapaxes, rollaxis
  • ravel, flatten
  • all other *stack ones
  • all *split ones
  • tile
  • repeat
  • block
  • delete, insert, append, resize
  • rot90
  • expand_dims
  • transpose

Of all those, the ones I'd at least consider for inclusion in addition to the ones in this PR are moveaxis, expand_dims and transpose.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 14, 2020

Transpose was added with the linear algebra functions. See here.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 21, 2020

Updates:

  • reshape: shape can now only be a tuple.
  • concat/stack: added output array data type guidance.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 21, 2020

Added expand_dims and updated the OP accordingly.

Re: moveaxis. Did not include this API in this round of inclusions, as this API does not appear to be present in either Torch or TensorFlow.

This PR should be ready for another round of review based on the aforementioned changes.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 24, 2020

I can provide a follow-up PR with manipulation functions which, while not universally implemented across array libraries, are commonly implemented and commonly used by downstream libraries (e.g., tile, repeat, etc).

@rgommers
Copy link
Member

rgommers commented Sep 28, 2020

I can provide a follow-up PR with manipulation functions which, while not universally implemented across array libraries, are commonly implemented and commonly used by downstream libraries (e.g., tile, repeat, etc).

I think it may make sense to open an issue for those with a summary, but wait with adding them to the standard until they're implemented universally. They're not all that important I'd say.

EDIT: or do nothing, not sure we need un-actionable issues right now.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Changes LGTM, and no further comments, so merging. Thanks @kgryte

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