Add triangular indices functions #4601

EPronovost · 2019-09-22T04:04:16Z

Add tril_indices, tril_indices_from, triu_indices, and triu_indices_from

Mirrors the implementation from numpy

…dices_from` Mirrors the implementation from numpy

EPronovost · 2019-09-22T04:08:06Z

Will need to update #4074 when merged

esc · 2019-09-23T08:42:41Z

@EPronovost thanks for implementing and submitting this. I have queued it for an initial review.

esc · 2019-09-23T13:12:19Z

FYI: CI errors are probably unrelated.

esc

Looks good so far! Some exception testing and corner cases will still need to be addressed.

numba/targets/arraymath.py

numba/tests/test_np_functions.py

numba/targets/arraymath.py

esc · 2019-09-23T13:59:34Z

@EPronovost thanks again for submitting this and welcome to Numba! I have reviewed your submission and it looks really good so far. There are a few things that still need to be addressed and I left some comments inline for your consideration. If anything is unclear or confusing, please don't hesitate to ask.

* Add explicit test for `n, m` arguments * Factored out test code for `triu_indices` * Fixed spacing for flake8

esc · 2019-09-26T08:11:58Z

@EPronovost thanks for making these changes and addressing the comments from the review, I will look at them today. Just a word of warning, I saw that you have force pushed the branch, probably due to a git rebase or similar operation. We have had some bad experiences with GitHub loosing review comments and discussions because it can no longer assign them to the respective source lines so we generally discourage the use of rebase. Instead we generally tend to recommend merging master into feature branches to update the code base and fix conflicts.

esc

This is in great shape, probably just a few more tidbits to fix, getting close to being mergeable.

esc · 2019-09-26T08:16:44Z

numba/targets/arraymath.py

+@register_jitable
+def _check_is_integer(v, name):
+    if not isinstance(v, (int, types.Integer)):
+        raise TypeError('{} must be an integer'.format(name))


We generally tend to use TypingError in these cases rather than TypeError. This is to indicate that Numba failed during type inference.

esc · 2019-09-26T08:19:45Z

numba/tests/test_np_functions.py

+
+def tril_indices_n_k_m(n, k=0, m=None):
+    return np.tril_indices(n, k, m)
+


There is one last corner case that you should address:

def tril_indices_n_k_m(n, k=0, m=None): return np.tril_indices(n, k=k, m=m)

I.e. supplying the arguments as keyword arguments. I recently discovered that this might lead to slightly different path and should be checked to be safe.

esc · 2019-09-26T08:25:26Z

numba/tests/test_np_functions.py

+
+        with self.assertTypingError() as raises:
+            cfunc(4, k=1.5)
+        assert "k must be an integer" in str(raises.exception)


If possible, can you add some code to test the exceptions for m and n too?

Also, please use self.assertIn(), Numba employs unittest style testing.

Use TypingError instead of TypeError to indicate that the error happened during TypeInference. Also. use unittest style testing with assertIn.

The ndim attribute is already available in the typing context. Removing an additional if-branch in the implementation is cleaner and will most likely accelerate the function a little, especially when called in a tight loop.

esc · 2019-09-27T10:07:33Z

@EPronovost FYI: we would like to get this PR in to the next RC. I have tended to this PR and pushed some commits to your branch so that it has a chance of making it into the next release.

stuartarchibald

@esc thanks for the fixes, I've added some notes/comments for you.

stuartarchibald · 2019-09-27T10:17:34Z

numba/targets/arraymath.py

@@ -614,6 +614,12 @@ def check_array(a):
        raise ValueError('zero-size array to reduction operation not possible')


+@register_jitable


why is this being compiled? the type is known at typing time?

stuartarchibald · 2019-09-27T10:18:11Z

numba/targets/arraymath.py

+    # we require integer arguments, unlike numpy
+    _check_is_integer(n, 'n')
+    _check_is_integer(k, 'k')
+    if m is not None:


This is a numba.numpy_support.is_nonelike case, None literal vs NoneType. Merge in master to get that function from it's new API home.

This ought to work:

from numba import njit import numpy as np @njit def triu_indices_n(n, k=0, m=None): return np.triu_indices(n, k, m) print(triu_indices_n(2)) print(triu_indices_n.py_func(2))

something like (but with the import at the top of file) will likely sort it:

from numba.numpy_support import is_nonelike if not is_nonelike(m): _check_is_integer(m, 'm')

same argument applies to the tril case.

yes, the tests don't actually capture this case..

stuartarchibald · 2019-09-27T10:31:56Z

numba/tests/test_np_functions.py

+
+        for dtype in [int, float, bool]:
+            for n in range(10):
+                for m in range(10):


itertools.product!?

stuartarchibald · 2019-09-27T10:33:14Z

numba/tests/test_np_functions.py

+                    self.assertEqual(type(expected), type(got))
+                    self.assertEqual(len(expected), len(got))
+                    for e, g in zip(expected, got):
+                        np.testing.assert_array_equal(e, g)


I'd assume that the arrays returned would be int type so self.assertPreciseEqual perhaps could be used?

I tried this, and the strides turned out to be different:

FAIL: test_tril_indices_from (numba.tests.test_np_functions.TestNPFunctions) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/vhaenel/git/numba/numba/tests/test_np_functions.py", line 1288, in test_tril_indices_from self._triangular_indices_from_tests_arr(tril_indices_from_arr) File "/Users/vhaenel/git/numba/numba/tests/test_np_functions.py", line 1221, in _triangular_indices_from_tests_arr self.assertPreciseEqual(e, g) File "/Users/vhaenel/git/numba/numba/tests/support.py", line 270, in assertPreciseEqual self.fail("when comparing %s and %s: %s" % (first, second, failure_msg)) AssertionError: when comparing [0 1] and [0 1]: Lists differ: [2.0] != [1.0] First differing element 0: 2.0 1.0 - [2.0] ? ^ + [1.0] ? ^ : different strides

stuartarchibald · 2019-09-27T10:34:13Z

numba/tests/test_np_functions.py

+    def _triangular_indices_exceptions(self, pyfunc):
+        cfunc = jit(nopython=True)(pyfunc)
+
+        # Exceptions leak references


this should be resolvable at typing time?

stuartarchibald · 2019-09-27T10:34:25Z

numba/tests/test_np_functions.py

+
+        for dtype in [int, float, bool]:
+            for n in range(10):
+                for m in range(10):


itertools.product!?

stuartarchibald · 2019-09-27T10:38:35Z

numba/tests/test_np_functions.py

+        for dtype in [int, float, bool]:
+            for n in range(10):
+                for m in range(10):
+                    arr = np.ones((n, m), dtype)


this is just testing matrices that are full? given the alg, perhaps put in some zeros via a slice with step or similar?!

You mean randomly or with structure?

don't think it matters, whatever is convenient.

actually, ignore this, am remembering a different function!

The type-checking does not need to be compiled. Also, this means potentially more flexibility.

* master: (54 commits) Add __init__.py files to ensure `numba/cext` directory is installed. Move get_extension_libs() into `numba/cext/__init__.py`. Fix doc build Add checks on the status of the dtor execution. Apply suggestions from code review Fix docs WRT new compiler pass infrastructure. Respond to review Update arraymath.py Fix numba#4589. Just pass NULL for b2d_func for constant dynamic sharedmem per block. No need for cfunc. Fix test Turn off verbose Fix missing int64_t on w32 Update numpysupported.rst Add checks Ensure include_path() returns an absolute path Turn on verbose to debug Add missing nrt.py file Add docs Add intrinsic that calls NRT_get_api() Fix py2 problem with Py_hash_t Rename to NRT_get_api, add method get_data, refactor test ...

When calling a function like: ``` @njit def func(a, b=None): # code ``` We also need to call it like: ``` func(1) ``` To make sure that we don't reject a plain None when b is a NoneType. In this case, this test was missing, but could be fixed by using `is_nonlike` to perform typechecking.

Instead of two nested for loops.

Need to test the type check for n and m too.

esc · 2019-09-27T13:50:36Z

Canceled build, will update with new changes shortly.

esc · 2019-09-27T14:10:01Z

@stuartarchibald can you give it another quick look? I think it's almost ready for 'waiting on CI'.

stuartarchibald

Thanks for the fixes.

EPronovost · 2019-09-29T21:17:53Z

It seems the tests are failing for Python 2.7 because inspect.signature is not defined. Have you dealt with this in other places?

stuartarchibald · 2019-09-30T08:38:43Z

@esc please could you take a look at the Py2.7 issue, there might be something to help in numba.six. Thanks.

As title.

Add tril_indices, tril_indices_from, triu_indices, and `triu_in…

5614074

…dices_from` Mirrors the implementation from numpy

esc added the 3 - Ready for Review label Sep 23, 2019

stuartarchibald requested a review from esc September 23, 2019 13:09

esc requested changes Sep 23, 2019

View reviewed changes

esc added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Sep 23, 2019

sklam added this to the Numba 0.46 RC milestone Sep 23, 2019

EPronovost added 2 commits September 23, 2019 22:16

Factor out integer type check

36df641

Improved test coverage for triu_indices

2993966

* Add explicit test for `n, m` arguments * Factored out test code for `triu_indices` * Fixed spacing for flake8

EPronovost force-pushed the epronovost/triangle-indices branch from 4f5159a to 2993966 Compare September 26, 2019 03:38

esc requested changes Sep 26, 2019

View reviewed changes

stuartarchibald mentioned this pull request Sep 26, 2019

Numba 0.46.0RC1 checklist #4622

Closed

32 tasks

esc added 2 commits September 27, 2019 10:32

fixup tests

8130b1b

Use TypingError instead of TypeError to indicate that the error happened during TypeInference. Also. use unittest style testing with assertIn.

move dimensionality check out of implementation

b2843d2

The ndim attribute is already available in the typing context. Removing an additional if-branch in the implementation is cleaner and will most likely accelerate the function a little, especially when called in a tight loop.

stuartarchibald reviewed Sep 27, 2019

View reviewed changes

esc added 5 commits September 27, 2019 14:06

remove register_jitable from type checking function

e7d6a09

The type-checking does not need to be compiled. Also, this means potentially more flexibility.

use itertools.product

82ed8d0

Instead of two nested for loops.

adding tests for parameter type checks

277aa55

Need to test the type check for n and m too.

esc approved these changes Sep 27, 2019

View reviewed changes

stuartarchibald approved these changes Sep 27, 2019

View reviewed changes

stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on author Waiting for author to respond to review labels Sep 27, 2019

fix for Python 2.7

b04ba2d

As title.

stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on CI Review etc done, waiting for CI to finish labels Sep 30, 2019

stuartarchibald merged commit 9a49a69 into numba:master Oct 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add triangular indices functions #4601

Add triangular indices functions #4601

EPronovost commented Sep 22, 2019

EPronovost commented Sep 22, 2019 •

edited

esc commented Sep 23, 2019

esc commented Sep 23, 2019

esc left a comment

esc commented Sep 23, 2019

esc commented Sep 26, 2019

esc left a comment

esc Sep 26, 2019

esc Sep 26, 2019

esc Sep 26, 2019

stuartarchibald Sep 26, 2019

esc commented Sep 27, 2019

stuartarchibald left a comment

stuartarchibald Sep 27, 2019

stuartarchibald Sep 27, 2019

esc Sep 27, 2019

stuartarchibald Sep 27, 2019

esc Sep 27, 2019

stuartarchibald Sep 27, 2019

esc Sep 27, 2019

stuartarchibald Sep 27, 2019

esc Sep 27, 2019

stuartarchibald Sep 27, 2019

esc Sep 27, 2019

stuartarchibald Sep 27, 2019

esc Sep 27, 2019

stuartarchibald Sep 27, 2019

stuartarchibald Sep 27, 2019

esc commented Sep 27, 2019

esc commented Sep 27, 2019

stuartarchibald left a comment

EPronovost commented Sep 29, 2019

stuartarchibald commented Sep 30, 2019


		def tril_indices_n_k_m(n, k=0, m=None):
		return np.tril_indices(n, k, m)

		@@ -614,6 +614,12 @@ def check_array(a):
		raise ValueError('zero-size array to reduction operation not possible')


		@register_jitable

Add triangular indices functions #4601

Add triangular indices functions #4601

Conversation

EPronovost commented Sep 22, 2019

EPronovost commented Sep 22, 2019 • edited

esc commented Sep 23, 2019

esc commented Sep 23, 2019

esc left a comment

Choose a reason for hiding this comment

esc commented Sep 23, 2019

esc commented Sep 26, 2019

esc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esc commented Sep 27, 2019

stuartarchibald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esc commented Sep 27, 2019

esc commented Sep 27, 2019

stuartarchibald left a comment

Choose a reason for hiding this comment

EPronovost commented Sep 29, 2019

stuartarchibald commented Sep 30, 2019

EPronovost commented Sep 22, 2019 •

edited