Skip to content

Add specifications for array creation functions #31

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 14, 2020
Merged

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Sep 7, 2020

This PR

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

Notes

  • This list of array creation functions is an initial set of array creation 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 regarding particular APIs...

    • eye: Torch and TensorFlow don't currently support k keyword argument for off-diagonal.

    • full: TensorFlow doesn't support dtype. Infers from fill_value.

    • full_like: Torch and MXNet support an out keyword argument.

    • linspace: Torch and TensorFlow don't support returning the step along with the samples. TensorFlow infers dtype from start. Torch and TensorFlow don't support endpoint keyword arg. Torch/CuPy/dask.array does not support axis keyword arg.

    • ones: NumPy default dtype is float64, while TensorFlow is float32.

    • ones_like: shape keyword argument for overriding shape is not universal and seems mostly intended for generating an array having the same dtype, but not the same shape.

    Note: some of the comments apply to multiple APIs (e.g., ones, zeros and ones_like, zeros_like, etc.

Questions

  • How to specify/spec dtype?

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.

Most choices for the inconsistencies you flagged seem fine to me.

linspace: Torch and TensorFlow don't support endpoint keyword arg.

Including endpoint in the signature here is the one choice I'd probably make differently. It's rarely used I think, and if both PyTorch and TensorFlow don't have the keyword, it may be better to not include it.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 10, 2020

@rgommers Thanks for the review!

Re: linspace endpoint. Based on record data, endpoint does appear to be used by downstream libraries. I suppose one argument in favor of including endpoint is that it makes creating half-open intervals easier when wanting to specify the number of samples, rather than the step (via arange).

Re: arange. To confirm, we are fine with parameter order for this interface? Currently, the interface is effectively an overload. In a more ideal world, we would make stop the first positional parameter and start an optional keyword argument. Maybe this ship has sailed, but wanted to bring it up for future reference.

@rgommers
Copy link
Member

How to specify/spec dtype?

This is addressed in gh-32

Re: linspace endpoint. Based on record data, endpoint does appear to be used by downstream libraries. I suppose one argument in favor of including endpoint is that it makes creating half-open intervals easier when wanting to specify the number of samples, rather than the step (via arange).

Thanks. Okay, I agree with keeping it. Nice to have data:)

Re: arange. To confirm, we are fine with parameter order for this interface? Currently, the interface is effectively an overload. In a more ideal world, we would make stop the first positional parameter and start an optional keyword argument. Maybe this ship has sailed, but wanted to bring it up for future reference.

Yeah, I think the ship has sailed, too heavily used so can't change it. Also, it's also consistent with the builtin range.

@rgommers
Copy link
Member

@kgryte what's here is ready right? PR says WIP, but that's because you wanted to add more functions? Maybe we should merge this as is soon, and in a second PR list add (if needed) other array creation functions as well as list the ones that were left out.

@kgryte kgryte changed the title WIP: Add specifications for array creation functions Add specifications for array creation functions Sep 14, 2020
@kgryte
Copy link
Contributor Author

kgryte commented Sep 14, 2020

@rgommers Correct. Should be ready.

I've updated the dtype keyword typings to something akin to the way array is indicated (i.e., as an implementation-dependent object).

And yes, there are a few additional APIs to add. The main one being meshgrid. But I'll plan on adding that one in a follow-up PR.

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.

Latest update LGTM, in it goes. Thanks @kgryte

@rgommers rgommers merged commit 2244ea1 into master Sep 14, 2020
@rgommers rgommers deleted the array-creation branch September 14, 2020 09:29
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