Enable support for count() method for unicode string #4204

rdesai16 · 2019-06-19T21:38:54Z

enables support for count() method for unicode string

stuartarchibald · 2019-06-20T09:34:43Z

Thanks for the PR. Was wondering why there are two merges included in this patchset? I think
82b7e57 just needs applying to the current numba master branch and that should be fine (I assume that's the only commit adding new code?). I think the root cause of these git issues is that the master branch of your fork of Numba is not tracking numba/master correctly, as can be seen by PRs https://github.com/rdesai16/numba/pulls?q=is%3Apr+is%3Aclosed that create new history via merges to sync opposed to just syncing the history directly. As a result, when you branch from your master branch you get those extra merge commits and then adding a new patch and opening a PR leads to them appearing as a difference in the history against numba/master. I'd strongly recommend fixing this ahead of opening any more pull requests.

rdesai16 · 2019-06-20T15:38:45Z

Thank you @stuartarchibald for the comments. As far as I understand the two merge requests were to sync my forked repo with original numba repo and then sync this branch my forked repo. But you're right. My forked repo wasn't in sync with original numba repo and that is why the old count method PR had issues in it. I made it right by updating my forked repo master branch. But if you feel like there are still some issues with this PR then let me know.

rdesai16 · 2019-06-27T18:02:54Z

@stuartarchibald This PR is ready for review in my opinion.

stuartarchibald · 2019-06-27T18:32:26Z

@rdesai16 thanks. The git history is still not right but the patch 82b7e57 can be reviewed.

rdesai16 · 2019-06-27T18:34:14Z

Thanks @stuartarchibald for your quick response. I will clear the git history but meanwhile the patch can be reviewed on its own.

esc

Overall this is a good start, but the code will need a number of stylistic and algorithmic improvements before it is ready to merge.

numba/unicode.py

esc · 2019-07-05T16:23:15Z

numba/tests/test_unicode.py

+                                 cfunc(sub, s),
+                                 "'%s' in '%s'?" % (sub, s))
+
+    def test_count_with_startend(self):


for readability, please rename this to test_count_with_start_end.

numba/tests/test_unicode.py

numba/unicode.py

numba/tests/test_unicode.py

esc · 2019-07-05T18:15:22Z

numba/unicode.py

+            else:
+                begin = start
+                new_end = e
+            if (begin < 0 and new_end < 0 and begin < new_end):


The next section deals with normalizing and clipping the start and end arguments. For simplicity you should probably do that first for each argument separately and you can define a helper function to do so. So for example, if the start argument is negative, take src_length and add the negative argument to it, if it then is still negative , clip it to zero. If it is positive on the other hand and beyond/larger than src_len, clip it to src_len. A similar approach can be taken for the end argument, and using the helper function will help to reduce overall code length. After this, you will have a valid range/slice in which you will search for the substring. At this stage you can and should also check for early exit conditions, such as the range/slice being zero, the length of the substring being longer than the range/slice and so on (there are a few more). Then, if everything is fine, commence with the substring search proper. This approach should also help reduce the number of cascaded if statements and make the code more readable overall.

I understand your suggestion and I have tried to make some changes to consider that. But the issue is that the algorithm is complex and less readable because of the slicing, bound checking conditions and python characteristic to give an answer without error even if the bounds(start,end args) are not correct or feasible for a range. Another issue with creating a helper function is that if you look at the algorithm carefully you see there are certain conditions that need to be checked in the main function for slicing or for returning count value and this complicates separating out some code into a helper function. So I feel that instead of changing the algorithm I can add more comments to make the code readable. Let me know what you think about that.

I think the implementation could work as follows:

Normalise the slice starts and ends to concrete integer values

Check if the slice start is in bounds else return 0

Slice the input string based on the normalised slice

Deal with empty input string and empty substring cases

Walk the sliced string using a sliding window of the length of the substring (use a slice?!) to check for matches and either bump the window start position by 1 (no match) or window length (match), accumulate the match count and return this.

Hope this helps.

@stuartarchibald I have tried to simplify the code to make it more readable in my new changes. Can you review that and see if that looks good? If it doesn't then I'll try simplifying using the algorithm you suggested.

esc · 2019-07-05T18:24:09Z

@seibert we are getting very close to the next release candidate (0.45.0) -- I will punt this to the next release (0.46.0) because it will probably take some more time to get this into shape.

rdesai16 · 2019-07-15T16:41:07Z

Thank you @esc for the review comments. All the comments are helpful and make sense. So I will make suggested changes and submit it again soon.

rdesai16 · 2019-07-17T14:51:46Z

@esc I have made changes as per suggestions. It is ready for review.

esc · 2019-07-19T12:48:51Z

To fix the history, we will soon force push this.

rdesai16 · 2019-07-19T13:29:30Z

@stuartarchibald I have used force push to fix the git history. So this PR can be reviewed further now.

esc

I had another look at this today and here is a simpler implementation that I managed to come up with:

@register_jitable
def normalize_slice_arg(arg, slice_length, default):
    if arg is None:
        return default
    elif arg < 0:
        return max(slice_length + arg,  default)
    else:
        return min(slice_length, arg)


@overload_method(types.UnicodeType, 'count')
def unicode_count(src, sub, start=None, end=None):

    if not (start is None or isinstance(start, (types.Omitted,
                                                types.Integer,
                                                types.Optional))):
        raise TypingError("Start arg must be of type Integer or None")

    if isinstance(sub, types.UnicodeType):
        def count_impl(src, sub, start=start, end=end):
            count, src_len, sub_len = 0, len(src), len(sub)

            start = normalize_slice_arg(start, src_len, 0)
            end = normalize_slice_arg(end, src_len, src_len)

            # Early exit if sub length is zero
            if sub_len == 0:
                return end - start + 1

            i = start
            while(i + sub_len <= end):
                if src[i:i + sub_len] == sub:
                    count += 1
                    i += sub_len
                else:
                    i += 1
            return count
        return count_impl

As far as I can tell, this passes all of the unit tests, let me know what you think and if there are any additional bugs that I haven't spotted yet.

Also, I think it would be good to have an 'overlapping' test. So for example given the string 'aaaa' and the substring 'aa' we need to find exactly two matches, whereas a naive implementation might find three.

esc · 2019-07-22T10:51:51Z

numba/tests/test_unicode.py

+        for s in UNICODE_EXAMPLES:
+            extras = ['', ' ', 'xx', s[::-1], s[:-2], s[3:], s, s + s]
+            for sub in [x for x in extras]:
+                self.assertEqual(pyfunc(sub, s),


I believe sub and s are in the wrong order here. This also applies to the other tests.

@esc Thanks for the code. I have tried this code and it passes all the tests except one where sub_string is empty. That part needs some change so I'll do that and resubmit it again.

Oh, that is good, which test was failing? I didn't notice any failing tests. Did this happen after changing the order or sub and s.

Yes it happened after changing the order for sub and s. It was failing for a case when the slice_range is bigger than src_len. But apart from that the algorithm is really useful because the same will be used in rfind method as well.

esc · 2019-07-23T08:53:15Z

numba/unicode.py

+
+    if not (start is None or isinstance(start, (types.Omitted, types.Integer, types.Optional))):
+        raise TypingError("Start arg must be of type Integer or None")
+


I think you will need to type check the end arg here too.

stuartarchibald

@rdesai16 Thanks for the updates. Given this now contains some of @esc's code I'll continue the review. General feedback is that:

This is much improved, good job with the fixes!
The type checking needs some work, I've made some suggestions and written a code sample for your perusal.
The tests could do with some further work to make them more suitable for testing the function proposed, suggestions are made inline.

Thanks again for your work on this.

stuartarchibald · 2019-07-23T08:10:37Z

numba/tests/test_unicode.py

+    return x.count(y)
+
+
+def count_with_start_end_usecase(x,y,start,end):


code style: spaces after commas

stuartarchibald · 2019-07-23T08:10:41Z

numba/tests/test_unicode.py

+    return x.count(y, start, end)
+
+
+def count_with_start_only_usecase(x,y,start):


code style: spaces after commas

stuartarchibald · 2019-07-23T08:18:07Z

numba/unicode.py

+            end = normalize_slice_arg(end, src_len, src_len)
+
+            if (sub_len == 0):       #special case when substring is empty
+                return _count_special_case(start, end, src_len)


Is this function just something like: len(src[slice(start, end)]) + 1 ?

Also, does this need to be a function?

@stuartarchibald Are you asking about _count_special_case? If yes then it needs to be a function because it has multiple conditions so including that in a function makes the code more readable.

It is similar to len(src[slice(start, end)]) + 1 but not exactly like that so I think it will be better to put it like this in a separate function. Python has unique behavior when it comes to using such methods with empty strings and more than one condition is needed to cover all the corner cases related to this special case. But if you have other suggestions regarding this I am open for that.

I think that this is because the src string isn't pre-sliced so the computation has to happen. Could the function body just be inlined here, it's small and only called from this location?

Okay will do that.

stuartarchibald · 2019-07-23T08:25:14Z

numba/unicode.py

+@overload_method(types.UnicodeType, 'count')
+def unicode_count(src, sub, start=None, end=None):
+
+    if not (start is None or isinstance(start, (types.Omitted, types.Integer, types.Optional))):


Not sure that this type check is quite right. Perhaps something like this would help:

def check_kwargs(ty, name): thety = ty if isinstance(ty, (types.Omitted)): # if the type is omitted, the concrete type is the value thety = ty.value elif isinstance(ty, (types.Optional)): # if the type is optional, the concrete type is the captured type thety = ty.type accepted = (types.Integer, types.NoneType, int) # classes that are accepted if not (thety is None or isinstance(thety, accepted)): # check if the resolved type is None or an accepted type msg = "Argument '{}' must be of type Integer or None" raise TypingError(msg.format(name)) check_kwargs(start, "start") # check start arg check_kwargs(end, "end") # check end arg

this is still not ideal because the Python implementation does not have start and end as kwargs, seems like they are positional only args, but there's nothing that can be done about this here.

Unit tests will need writing to ensure that the type rejection logic is correct.

Sorry for my confusion but I am bit confused here. So are you suggesting that we should have a separate function for typechecking and then separate unit test for testing that function?

@stuartarchibald we have several other functions in unicode.py that do typechecking in similar ways. So in my opinion if we create a separate function then it should be flexible enough so that it can be used everywhere or we can keep it this way to maintain code format. what do you think?

I think that if it is going to be used repeatedly, pulling the function out to module level makes sense.

Sorry for my confusion but I am bit confused here. So are you suggesting that we should have a separate function for typechecking and then separate unit test for testing that function?

I do not mind how the type checking is implemented, but it seems like a separate function would be useful as exactly the same checks are needed for start and end. There should be separate unit tests to check that compilation fails gracefully during type inference for unsupported/invalid types, an example is here:

numba/numba/tests/test_unicode.py

Lines 465 to 470 in 37ab142

def test_repeat_exception_float(self):

self.disable_leak_check()

cfunc = njit(repeat_usecase)

with self.assertRaises(TypingError) as raises:

cfunc('hi', 2.5)

self.assertIn('Invalid use of Function(<built-in function mul>)', str(raises.exception))

this checks the multiplication of a string by a floating point value fails during typing.

Hope this helps.

Oh okay! Got it. I'll make it in a separate function then.

@stuartarchibald I have a doubt about this test showed as an example. What does the following line do?
self.assertIn('Invalid use of Function()', str(raises.exception))

Can you explain briefly? Thanks!

self.assertIn('Invalid use of Function()', str(raises.exception))

this line is calling the assertIn function which asserts something contains something else, str(raises.exception) is converting the exception captured in the raises object yielded from the with self.assertRaises into a string so that it can be compared to an expected string. This is a convenient way of checking that the right class of exception was raised, along with the contents of the exception message being correct.

stuartarchibald · 2019-07-23T08:28:19Z

numba/tests/test_unicode.py

+        for s in UNICODE_EXAMPLES:
+            extras = ['', ' ', 'xx', s[::-1], s[:-2], s[3:], s, s + s]
+            for sub in [x for x in extras]:
+                for i , j in zip(range(-2,4), (0,6)):


This will yield the values:

-2 0 -1 6

was that intended?!

I expect that itertools.product would be of use here:

for i , j in itertools.product(range(-20, 20), range(-20, 20)):

Yes that is intended because I wanted to cover all kinds of cases in the test. I had thought about using itertools but I can't quite remember now why I ended up using zip. Does itertool have significant benefit over zip?

If the requirement is to just testing two values then perhaps just spell those out instead of using range? On the basis that it seems like testing more values would be useful, consider the following:

In [51]: for i, j in zip(range(-2,4), (0,6)): ...: print(i, j) ...: -2 0 -1 6 In [52]: for i, j in ((-2, 0), (-1, 6)): ...: print(i, j) ...: -2 0 -1 6 In [53]: for i, j in itertools.product(range(-3, 3), range(-2, 2)): ...: print(i, j) ...: -3 -2 -3 -1 -3 0 -3 1 -2 -2 -2 -1 -2 0 -2 1 -1 -2 -1 -1 -1 0 -1 1 0 -2 0 -1 0 0 0 1 1 -2 1 -1 1 0 1 1 2 -2 2 -1 2 0 2 1

Ooh I realize my mistake now! Sorry I had some misunderstanding. I hadn't realized until now that I should have used "range" in front of (0,6). I get what you're saying now. I'll fix it. Thanks!

I think that if you use two zipped ranges then the result will be monotonic.

In [56]: for i , j in zip(range(-2,4), range(0,6)): ...: print(i, j) ...: ...: -2 0 -1 1 0 2 1 3 2 4 3 5

whereas itertools.product will cover the space start > end, start == end, start < end with a variety of negative and positive values.

Yes you're right! Using itertools.product will cover good range. I will change that.

stuartarchibald · 2019-07-23T08:30:12Z

numba/tests/test_unicode.py

+        for s in UNICODE_EXAMPLES:
+            extras = ['', ' ', 'xx', s[::-1], s[:-2], s[3:], s, s + s]
+            for sub in [x for x in extras]:
+                for i in range(-2, 3):


The magnitude of the range here should be larger to encompass access before and after the valid range, this to accommodate checking with the longer unicode strings.

True. I'll change that.

stuartarchibald · 2019-07-23T09:19:47Z

numba/unicode.py

+    if arg is None:
+        return default
+    elif arg < 0:
+        return max( arg + slice_len, default)


Suggested change

return max( arg + slice_len, default)

return max(arg + slice_len, default)

oh thanks for bringing that to my notice! Will fix it

stuartarchibald · 2019-07-23T09:20:12Z

numba/unicode.py

@@ -381,6 +381,18 @@ def _find(substr, s):
            return i
    return -1

+# The following funciton handles special case for count method where substring is empty


Suggested change

# The following funciton handles special case for count method where substring is empty

# The following function handles special case for count method where substring is empty

Thanks again! Will fix it

stuartarchibald · 2019-07-23T09:20:41Z

numba/unicode.py

@@ -381,6 +381,18 @@ def _find(substr, s):
            return i
    return -1

+# The following funciton handles special case for count method where substring is empty
+@njit(_nrt=False)


This should be @register_jitable?

Yes it should be. Will change that.

stuartarchibald · 2019-07-23T09:24:43Z

numba/tests/test_unicode.py

+        pyfunc = count_usecase
+        cfunc = njit(pyfunc)
+
+        for s in UNICODE_EXAMPLES + ['aaaa']:


I'm not sure that UNICODE_EXAMPLES contains suitable material for this test? There's no zero length string, no repeats, no nested repeats. Perhaps take a look at the cpython tests for some inspiration around what would be good to check: https://github.com/python/cpython/blob/5623ac87bbe5de481957eca5eeae06347612fbeb/Lib/test/test_unicode.py#L177-L201

That's a good point! I will add some more tests form cpython add improve it.

rdesai16 · 2019-07-24T18:26:15Z

Thanks @stuartarchibald for your review. All the comments are very helpful. I have some doubts which I have mentioned in my comments. apart from that I will make necessary changes and submit it.

rdesai16 · 2019-07-30T19:47:10Z

@stuartarchibald @esc Ignoring the Travis CI and install errors I have made all the changes as per the comments. You can review it again now.

stuartarchibald

Thanks for the fixes, this is close to working now. However, adding more tests has exposed more issues that need addressing, please see the comments below. Most serious is that there seems to be an algorithmic bug.

Finally, the documentation will need updating to note that this feature has been implemented. The appropriate location is here: https://github.com/numba/numba/blob/81b91f93c60cd4bfe9d6c1cfce1191e64bdf2a5c/docs/source/reference/pysupported.rst#str

Thanks again.

stuartarchibald · 2019-08-05T13:14:14Z

numba/unicode.py

@@ -817,6 +872,13 @@ def unicode_strip_types_check(chars):
        raise TypingError('The arg must be a UnicodeType or None')


+def args_types_check(arg):


This function should probably be called _count_arg_type_check.

stuartarchibald · 2019-08-05T13:14:57Z

numba/tests/test_unicode.py

+            extras = ['', ' ', 'xx', s[::-1], s[:-2], s[3:], s, s + s, 'a', 'a_',
+                      '\u0102', 'a\u0102', 'a\U00100304', '\u0102_', '\u0102\U00100304']
+            for sub in [x for x in extras]:
+                for i , j in product(range(-10,10), (0,20)):


Suggested change

for i , j in product(range(-10,10), (0,20)):

for i , j in product(range(-10, 10), (0, 20)):

Also, might it make sense for these ranges extend symmetrically about zero and to the length of the longest test string+1 ? Same comment applies below to the start only case.

For example, right now, this fails and was not caught by this test:

from numba import njit @njit def foo(x, y): return x.count(y, -40, -40) a = "ascii" b = "" print(foo.py_func(a, b)) print(foo(a, b))

the implementation in this PR erroneously returns 6.

@stuartarchibald I am not clear about the range suggestion. So are you suggesting to have the range like (-20,20) or (0,20)?

stuartarchibald · 2019-08-05T13:32:20Z

numba/unicode.py

@@ -869,6 +872,13 @@ def unicode_strip_types_check(chars):
        raise TypingError('The arg must be a UnicodeType or None')


+def args_types_check(arg):
+    if not (arg is None or isinstance(arg, (types.Omitted,
+                                            types.Optional,


The type of the Optional should be checked to ensure it also is an integer type, for example:

from numba import njit @njit def foo(x, y): if len(x) > 14: maybe_none = 'oops' else: maybe_none = None return x.count(y, maybe_none, 1) a = "ascii" b = "a" print(foo.py_func(a, b)) print(foo(a, b))

fails strangely. One way to do this is something like if isinstance(arg, types.Optional) and not isinstance(arg.type, types.Integer: raise .... A test should be added to ensure this is correctly implemented.

stuartarchibald · 2019-08-05T13:35:29Z

numba/unicode.py

+def args_types_check(arg):
+    if not (arg is None or isinstance(arg, (types.Omitted,
+                                            types.Optional,
+                                            types.Integer))):


This should also accept types.NoneType for cases like:

from numba import njit @njit def foo(x, y, none_ty=None): return x.count(y, none_ty, 1) a = "ascii" b = "a" print(foo.py_func(a, b)) print(foo(a, b))

where start is a NoneType opposed to the literal None. A test should be added to ensure this is correctly implemented.

rdesai16 · 2019-08-05T17:28:36Z

Thanks @stuartarchibald for the review. I'll make the required changes and also add the documentation for it.

Vyacheslav-Smirnov · 2019-08-22T11:16:20Z

Hi,
Could you please set label "In Progress" for this PR?
I am going to continue work here.

esc · 2019-08-22T11:42:57Z

@Vyacheslav-Smirnov sounds good!

stuartarchibald · 2019-09-18T13:55:15Z

Thanks for writing this patch for Numba. This PR is now unfortunately stale with respect to the current master branch. If you wish to resubmit for review please do so by reopening this PR and rebasing the patch(es) against current master. Thanks for your help in improving Numba!

stuartarchibald added the 2 - In Progress label Jun 20, 2019

rdesai16 changed the title ~~[WIP]Enable support for count() method for unicode string~~ Enable support for count() method for unicode string Jun 27, 2019

stuartarchibald added 3 - Ready for Review and removed 2 - In Progress labels Jun 27, 2019

stuartarchibald self-requested a review July 2, 2019 15:57

seibert added this to the Numba 0.45 RC milestone Jul 2, 2019

esc requested changes Jul 5, 2019

View reviewed changes

esc modified the milestones: Numba 0.45 RC, Numba 0.46 RC Jul 5, 2019

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

rdesai16 added 2 commits July 19, 2019 07:48

count() method for unicode string

ebfd3ea

Changes as per review suggestions

88ddc83

rdesai16 force-pushed the count_string branch from a7268bd to 88ddc83 Compare July 19, 2019 12:50

esc requested changes Jul 22, 2019

View reviewed changes

added new simple algorithm

d4a0280

esc reviewed Jul 23, 2019

View reviewed changes

stuartarchibald requested changes Jul 23, 2019

View reviewed changes

stuartarchibald mentioned this pull request Jul 23, 2019

String rfind() method support #4154

Closed

added changes as per review comments

f28234b

stuartarchibald requested changes Aug 5, 2019

View reviewed changes

rdesai16 added 2 commits August 7, 2019 17:15

changed some code as per suggestions

e63d8b9

fixed failures with test_count_arg_type_check

72d2402

stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Aug 15, 2019

esc added 2 - In Progress and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Aug 22, 2019

stuartarchibald closed this Sep 18, 2019

Vyacheslav-Smirnov mentioned this pull request Sep 23, 2019

Enable support for count() method for unicode string #4606

Merged


		if not (start is None or isinstance(start, (types.Omitted, types.Integer, types.Optional))):
		raise TypingError("Start arg must be of type Integer or None")

		return x.count(y)


		def count_with_start_end_usecase(x,y,start,end):

		return x.count(y, start, end)


		def count_with_start_only_usecase(x,y,start):

	def test_repeat_exception_float(self):
	self.disable_leak_check()
	cfunc = njit(repeat_usecase)
	with self.assertRaises(TypingError) as raises:
	cfunc('hi', 2.5)
	self.assertIn('Invalid use of Function(<built-in function mul>)', str(raises.exception))

	return max( arg + slice_len, default)
	return max(arg + slice_len, default)

	# The following funciton handles special case for count method where substring is empty
	# The following function handles special case for count method where substring is empty

		@@ -817,6 +872,13 @@ def unicode_strip_types_check(chars):
		raise TypingError('The arg must be a UnicodeType or None')


		def args_types_check(arg):

	for i , j in product(range(-10,10), (0,20)):
	for i , j in product(range(-10, 10), (0, 20)):

Enable support for count() method for unicode string #4204

Enable support for count() method for unicode string #4204

Conversation

rdesai16 commented Jun 19, 2019

stuartarchibald commented Jun 20, 2019

rdesai16 commented Jun 20, 2019

rdesai16 commented Jun 27, 2019

stuartarchibald commented Jun 27, 2019

rdesai16 commented Jun 27, 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

Choose a reason for hiding this comment

esc commented Jul 5, 2019

rdesai16 commented Jul 15, 2019

rdesai16 commented Jul 17, 2019

esc commented Jul 19, 2019

rdesai16 commented Jul 19, 2019 • edited

esc left a comment • edited

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

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

rdesai16 Jul 24, 2019 • edited

Choose a reason for hiding this comment

rdesai16 Jul 25, 2019 • edited

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

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

rdesai16 commented Jul 24, 2019

rdesai16 commented Jul 30, 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

rdesai16 commented Aug 5, 2019

Vyacheslav-Smirnov commented Aug 22, 2019

esc commented Aug 22, 2019

stuartarchibald commented Sep 18, 2019

rdesai16 commented Jul 19, 2019 •

edited

esc left a comment •

edited

rdesai16 Jul 24, 2019 •

edited

rdesai16 Jul 25, 2019 •

edited