Skip to content

np.alen#4374

Closed
guilhermeleobas wants to merge 4 commits intonumba:masterfrom
guilhermeleobas:np-alen
Closed

np.alen#4374
guilhermeleobas wants to merge 4 commits intonumba:masterfrom
guilhermeleobas:np-alen

Conversation

@guilhermeleobas
Copy link
Copy Markdown
Contributor

This PR implements np.alen.

I didn't include any test using regular lists because the usage of reflection lists is being deprecated.

@guilhermeleobas
Copy link
Copy Markdown
Contributor Author

Not sure if the CI is failing because of changes I made

@esc
Copy link
Copy Markdown
Member

esc commented Jul 29, 2019

@guilhermeleobas I think it was just a glitch, I restarted the travis tests for you just now.

Copy link
Copy Markdown
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. Generally looks good but seems like a few more things are accepted by this function (see comments). Also, the documentation will need an update in this section: https://github.com/numba/numba/blob/37ab1420fa87d02b635991b32f0faf5c65e8fc4d/docs/source/reference/numpysupported.rst#other-functions

Thanks.

@overload(np.alen)
def np_alen(a):
if not type_can_asarray(a):
raise errors.TypingError("The argument to np.shape must be array-like")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The function being typed is np.alen not np.shape?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, np.alen seems to accept a very broad range of things...

In [12]: np.alen('aaa')                                                                                                         
Out[12]: 3

In [13]: np.alen(np)                                                                                                            
Out[13]: 1

In [14]: np.alen(np.alen)                                                                                                       
Out[14]: 1

some of which are very strange, I don't think the bizarre things are worth considering, however, strings and perhaps other types in numba's type system that can be accepted should be.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've implemented alen for strings.

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jul 29, 2019
@stuartarchibald stuartarchibald added this to the Numba 0.46 RC milestone Jul 29, 2019
@seibert
Copy link
Copy Markdown
Contributor

seibert commented Jul 29, 2019

Just out of curiosity, are there situations where you have seen np.alen() being used in the wild? After looking at the function, I'm having a hard time thinking of a situation in Python/NumPy where len() would fail, but np.alen() would succeed.

@seibert
Copy link
Copy Markdown
Contributor

seibert commented Jul 29, 2019

Actually, on further investigation, I don't see this method in the NumPy docs at all. Was this dropped from NumPy at some point?

@sklam
Copy link
Copy Markdown
Member

sklam commented Jul 29, 2019

@stuartarchibald
Copy link
Copy Markdown
Contributor

I don't think alen is used internally in NumPy, it's not documented and seems like it hasn't been touched since ~2005. https://github.com/numpy/numpy/blame/master/numpy/core/fromnumeric.py#L2758

@godaygo
Copy link
Copy Markdown

godaygo commented Jul 29, 2019

As I know it was introduced when from numpy import * idiom was popular and seemed useful to prevent builtin shadowing. Nowdays it is not encouraged to use it, but also it is not deprecated, and you can see it in some places in the docs: https://docs.scipy.org/doc/numpy/reference/generated/numpy.ma.shape.html.

@guilhermeleobas
Copy link
Copy Markdown
Contributor Author

I don't think alen is used internally in NumPy, it's not documented and seems like it hasn't been touched since ~2005. numpy/numpy:numpy/core/fromnumeric.py@master#L2758 (blame)

A quick search on GitHub shows that some people still use np.alen. Maybe it is not worth adding it to Numba if NumPy decides to deprecate it.

@stuartarchibald
Copy link
Copy Markdown
Contributor

Seems like it's now scheduled for deprecation over in NumPy numpy/numpy#14155 (comment), I think that this is probably the best outcome for everyone and suggest that we don't replicate a feature scheduled for deprecation. Are you ok with this outcome @guilhermeleobas? It was great that you put up this PR, but I think the overall outcome of helping clean up NumPy by forcing a query on the use of this function is a net positive outcome too.

@guilhermeleobas
Copy link
Copy Markdown
Contributor Author

Yes, no worries

@stuartarchibald
Copy link
Copy Markdown
Contributor

@guilhermeleobas thanks for your understanding, much appreciated. Closing.

@stuartarchibald stuartarchibald added no action required No action was needed to resolve. numpy and removed 4 - Waiting on author Waiting for author to respond to review labels Jul 29, 2019
@stuartarchibald stuartarchibald removed this from the Numba 0.46 RC milestone Jul 29, 2019
@guilhermeleobas guilhermeleobas deleted the np-alen branch February 8, 2020 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no action required No action was needed to resolve. numpy

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants