Skip to content

MAINT: Replace npyiter_order_converter with PyArray_OrderConverter#16009

Merged
seberg merged 1 commit intonumpy:masterfrom
eric-wieser:remove-npyiter-converter
Apr 27, 2020
Merged

MAINT: Replace npyiter_order_converter with PyArray_OrderConverter#16009
seberg merged 1 commit intonumpy:masterfrom
eric-wieser:remove-npyiter-converter

Conversation

@eric-wieser
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser commented Apr 17, 2020

Builds upon #16008, marking as draft until that is merged


The TODO precondition now holds, so we no longer need this function.function_base.py

A consequence of this change is that:

  • None is now accepted as an alias for 'K'
  • Lowercase letters are now accepted as a replacement for the corresponding uppercase letter

Neither of these are particularly desirable, but we could always deprecate them in PyArray_OrderConverter at a later date.

The TODO precondition now holds, so we no longer need this function.function_base.py

A consequence of this change is that:
* `None` is now accepted as an alias for `'K'`
* Lowercase letters are now accepted as a replacement for the corresponding uppercase letter

Neither of these are particularly desirable, but we could always deprecate them in PyArray_OrderConverter at a later date.
@eric-wieser eric-wieser force-pushed the remove-npyiter-converter branch from 5117119 to 6186544 Compare April 27, 2020 17:10
@eric-wieser eric-wieser marked this pull request as ready for review April 27, 2020 17:10
@eric-wieser
Copy link
Copy Markdown
Member Author

Are we ok with this relaxing the argument specification, or should I add a new PyArray_OrderConverterStrict, and call that from nditer?

@seberg
Copy link
Copy Markdown
Member

seberg commented Apr 27, 2020

Yeah, its nonsense to do it differently for one function, we could generally deprecate the lower case if we like instead (not sure its worth the noise)...
How come this was based on the other one, the other one should not actually have added K, that must have been very long ago, no?

@eric-wieser
Copy link
Copy Markdown
Member Author

The todo appears to have come from fe791db

@seberg
Copy link
Copy Markdown
Member

seberg commented Apr 27, 2020

Nvm, was just surprised for a bit before deciding that it doesn't really matter and clarifying that the other PR did not change behaviour/was not an actual prerequisite.

Looks all fine to me, just waiting for tests. Thanks.

@eric-wieser
Copy link
Copy Markdown
Member Author

Hmm. I think the other PR almost was a prequisite, but 10 days is too long for me to remember why.

@seberg
Copy link
Copy Markdown
Member

seberg commented Apr 27, 2020

I suppose there may be some corner cases... with the error type or so.

@eric-wieser
Copy link
Copy Markdown
Member Author

That's probably what it was.

@seberg seberg merged commit 05595e9 into numpy:master Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants