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

unravel_index() argument name dims is not consistent with other functions #10586

Closed
SnoopJ opened this issue Feb 14, 2018 · 1 comment
Closed

Comments

@SnoopJ
Copy link
Contributor

SnoopJ commented Feb 14, 2018

The unravel_index() function takes an argument named dims that corresponds to:

The shape of the array to use for unraveling indices.

It seems like in most other places, this argument would be named shape, so I propose to rename it to match that convention, unless there is some special reason for this name to be different.

@eric-wieser
Copy link
Member

eric-wieser commented Feb 14, 2018

I agree that shape is a better name, but we need to not break code that calls np.unravel_index(inds, dims=(...,)). The easiest way to do that in the C code is something like:

// pseudocode
if (ParseNewArgs(...) < 0) {
    save_current_exception();
    if (ParseOldArgs(...) < 0) {
        restore_exception();
        return NULL;
    }
    print_warning_about_old_argname();
}

Edit: This is a bad idea as it produces misleading error messages

tylerjereddy added a commit to tylerjereddy/numpy that referenced this issue Oct 16, 2018
* unravel_index() now supports the shape argument
(Fixes numpy#10586) while retaining backwards compatibility
for the dims argument

* added corresponding unit tests and docstring
changes

* updated compatibility release notes
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

No branches or pull requests

3 participants