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

Arguments need not be strictly positional or keyword-only for creation and manipulation functions #85

Closed
shoyer opened this issue Nov 11, 2020 · 3 comments · Fixed by #167

Comments

@shoyer
Copy link
Contributor

shoyer commented Nov 11, 2020

It makes sense to require positional only arguments for functions like add() where there are no meaningful names, and likewise to require keyword arguments for true options.

However, this is less useful for most creation and manipulation functions. For example, arange currently has the signature arange(start, /, *, stop=None, step=1, dtype=None), but readable code could pass both start and stop as either positional or keyword arguments, e.g., np.arange(start, stop) and np.arange(start=0, stop=10).

I would suggest revisiting all of these functions and allowing arguments to be positional and keyword based when appropriate. Here are my suggestions off-hand:

  • arange(start, /, *, stop=None, step=1, dtype=None) -> arange(start, stop=None, *, step=1, dtype=None)
  • empty(shape, /, *, dtype=None) -> empty(shape, dtype=None)
  • full(shape, fill_value, /, *, dtype=None) -> full(shape, fill_value, *, dtype=None)
  • linspace(start, stop, num, /, *, dtype=None, endpoint=True) -> linspace(start, stop, num, *, dtype=None, endpoint=True)
  • ones and zeros should match empty
  • expand_dims(x, axis, /) -> expand_dims(x, /, axis) or even expand_dims(x, /, *, axis) to match other manipulation functions
  • reshape(x, shape, /) -> reshape(x, /, shape)
  • roll(x, shift, /, *, axis=None) -> roll(x, /, shift, *, axis=None)
@asmeurer
Copy link
Member

arange is tricky because arange(n) with a single argument should apply to the stop (arange(start=n) wouldn't make any sense). I agree on the others.

@kgryte
Copy link
Contributor

kgryte commented Nov 12, 2020

A primary argument for the current conventions is consistency, which underpins one of the stated goals of the consortium.

By adopting consistent conventions across all defined interfaces we reduce the cognitive overhead required by readers, users, and developers to recall which interfaces support positional-only, keyword-only, and positional or keyword arguments.

Furthermore, by enforcing a singular approach to providing arguments, optional or otherwise, we ensure uniformity of downstream consumption. If certain functions abide by different rules, unless a consumer is intimately familiar with either the classification of an interface (element-wise, linalg, creation/manipulation, etc), a consumer would not know a priori whether an interface ought to support one convention over the other. A one-true-way approach avoids this overhead, which I'd prioritize over saving keystrokes or permitting increased flexibility.

Lastly, not clear how we'd define "true" options from "untrue" options. In short, the current conventions follow the dictum that, if an argument need not be required, either because of defaults or because providing such an argument results in secondary behavior (e.g., specialized use case), then an argument is optional and is thus keyword-only.

@rgommers
Copy link
Member

rgommers commented Nov 26, 2020

I agree too this needs another look, to make sure we don't forbid writing clear code (e.g. using num as a keyword really helps)

empty(shape, /, *, dtype=None) -> empty(shape, dtype=None)

That should be either left as is or changed to empty(shape, *, dtype=None) I think. dtype as keyword-only is desired.

I actually think writing empty(shape=(N,), ...) is quite uncommon, so leaving as is would be fine with me.

or even expand_dims(x, /, *, axis) to match other manipulation functions

Making all axis arguments keyword-only too sounds good to me.

A primary argument for the current conventions is consistency, which underpins one of the stated goals of the consortium.

I think consistency is important, but not the only concern - in cases where we break the most common or clearest way of writing code right now, we should fix that.

rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit to rgommers/array-api that referenced this issue Apr 20, 2021
rgommers added a commit that referenced this issue Apr 27, 2021
* Update specification for arange

Addresses comments in gh-85 and gh-107

* Update the specification for `full` and `full_like`

Addresses comments in gh-85 and gh-107

* Update specification for `linspace`

Addresses comments in gh-85 and gh-107

* Update specification for `empty`, `ones`, `zeros`

Addresses comments in gh-85 and gh-107

* Update specification for `eye`

This is useful/needed because `M` is not a descriptive name
and that name does not match between different array libraries.

* Update specification for `expand_dims`, `roll` and `reshape`

Address comment in gh-85

* One more change to `eye`, more descriptive positional arguments

* Address the default integer dtype issue for 32/64-bit Python

Closes gh-151

* Update signature of `broadcast_to`

Address a review comment; makes it consistent with other functions
using `shape`.
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 a pull request may close this issue.

4 participants