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

Support len() of multi-dim arrays in array analysis #4239

Merged
merged 3 commits into from Jul 4, 2019

Conversation

ehsantn
Copy link
Collaborator

@ehsantn ehsantn commented Jun 28, 2019

As title. The check for 1D array in len handling of array analysis is unnecessary.

@esc
Copy link
Member

esc commented Jun 30, 2019

This looks good, thank you for the patch. One tiny nitpick: you have used the 'blessed' repo numba/numba to store the branch for this pull-request. In general, we aim to use our own forks to store branches for pull-requests because this means the blessed repo stays clean and any fresh forks don't inherit development/feature branches. Here is a listing of the current branches for numba/numba:

💥 zsh» git branch -a | grep remotes/numba                                                                                                                          
  remotes/numba/0.11_maintenance
  remotes/numba/0.38-patches
  remotes/numba/HEAD -> numba/master
  remotes/numba/gh-pages
  remotes/numba/master
  remotes/numba/master_30_april_2019
  remotes/numba/multidim_len_analysis
  remotes/numba/newtargets
  remotes/numba/ocl-compiler
  remotes/numba/ocl-driver
  remotes/numba/release0.19.2
  remotes/numba/release0.26.0
  remotes/numba/release0.30.1
  remotes/numba/release0.40
  remotes/numba/release0.41
  remotes/numba/release0.42
  remotes/numba/release0.43
  remotes/numba/release0.43.1
  remotes/numba/release0.44

@ehsantn
Copy link
Collaborator Author

ehsantn commented Jun 30, 2019

@esc looks like there was a problem with my remotes in this case. I'll delete the branch as soon as it's merged.

Copy link
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, change seems good, wondering if the test can be extended opposed to altered (see comment). Thanks again!

@@ -354,7 +354,7 @@ def test_12():
c = a[1:,:]
d = b[:-1,:]
e = c.shape[0]
f = d.shape[0]
f = len(d)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this could be g = len(d) and then use it in the return, and then assert equivalence over 'e', 'f', 'g' so as to still retain the original check based on shapes and slices in the 2D array context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@stuartarchibald stuartarchibald added the 4 - Waiting on author Waiting for author to respond to review label Jul 1, 2019
@stuartarchibald stuartarchibald added this to the Numba 0.45 RC milestone Jul 1, 2019
@seibert
Copy link
Contributor

seibert commented Jul 2, 2019

Also, merging master into this PR will fix the flake8 failure.

@ehsantn
Copy link
Collaborator Author

ehsantn commented Jul 3, 2019

Thank you @stuartarchibald and @seibert . Applied the changes.

Copy link
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 fixes.

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Jul 3, 2019
@stuartarchibald
Copy link
Contributor

I've marked this for merge, the only fails on CI were due to 504's on platforms that don't support ParallelAccelerator. CI has been restarted.

@seibert seibert merged commit 6178708 into master Jul 4, 2019
@ehsantn ehsantn deleted the multidim_len_analysis branch July 4, 2019 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants