stats: invalid usage of for enumerate and else clause unconditionally logs "Negative bucket boundaries" and trims off 0 boundary values

[odeke-em]

Currently in this code there is code

opencensus-python/opencensus/stats/aggregation.py

Lines 140 to 149 in f814904

	for ii, bb in enumerate(boundaries):
	if bb > 0:
	break
	else:
	ii += 1
	if ii:
	logger.warning("Dropping {} negative bucket boundaries, the "
	"values must be strictly > 0"
	.format(ii))
	boundaries = boundaries[ii:]

that uses for/else but we mistakenly assume that the else will ONLY be executed if we encounter a break. However as per https://docs.python.org/3/tutorial/controlflow.html

I believe tries to ensure that the bucket boundaries are > 0. However, there are two subtle bugs:
a) if a bucket boundary starts at 0 e.g. [0, 5, 10, 200], it will always falsely trim the list to [5, 10, 200] but 0 isn't a negative value
b) for and then else will ALWAYS run the else clause whenever the for loop terminates so still we falsely report that 1 value is getting dropped

Illustration of the 2 bugs

To illustrate please take a look at this code below:

#!/usr/bin/env python

def trim(boundaries):
 for ii, bb in enumerate(boundaries):
     if bb > 0:
         break
 else:
     ii += 1

 if ii <= 0:
     return None, boundaries
     
 return boundaries[:ii], boundaries[ii:]

def main():
    samples = [
            [0, 10, 5],
            [1, 2, 5],
            [-1, -2, -5],
    ]

    for sample in samples:
        with_negatives, without_negatives = trim(sample)
        print("original: %s\nwith_negatives:    %s\nwithout_negatives: %s\n\n"%(
            sample, with_negatives, without_negatives))

if __name__ == '__main__':
    main()

which produces

original: [0, 10, 5]
with_negatives:    [0]
without_negatives: [10, 5]


original: [1, 2, 5]
with_negatives:    None
without_negatives: [1, 2, 5]


original: [-1, -2, -5]
with_negatives:    [-1, -2, -5]
without_negatives: []

I think we can definitely fix this code with clarity and less else magic

first_non_negative_elem_index = -1
for i, boundary in enumerate(boundaries): 
     if boundary >= 0: 
         first_non_negative_elem_index = i
         break

if first_non_negative_elem_index > 0:
     logger.warning("Dropping {} negative bucket boundaries, the " 
                    "values must be strictly > 0" 
                    .format(first_non_negative_elem_index)) 
     boundaries = boundaries[first_non_negative_elem_index:] 

Full illustration

#!/usr/bin/env python

def proper_trim(boundaries):
 first_non_negative_index = -1
 for ii, bb in enumerate(boundaries):
     if bb >= 0:
         first_non_negative_index = ii
         break

 if first_non_negative_index < 0:
     return boundaries, None
     
 ii = first_non_negative_index
 return boundaries[:ii], boundaries[ii:]

def trim(boundaries):
 for ii, bb in enumerate(boundaries):
     if bb > 0:
         break
 else:
     ii += 1

 if ii <= 0:
     return None, boundaries
     
 return boundaries[:ii], boundaries[ii:]

def main():
    samples = [
            [0, 10, 5],
            [1, 2, 5],
            [-1, -2, -5],
    ]

    fns = [('buggy_trim', trim), ('fixed_trim', proper_trim)]
    for sample in samples:
        for name, fn in fns:
            with_negatives, without_negatives = fn(sample)
            print("%s        (%s)\nwith_negatives:    %s\nwithout_negatives: %s\n"%(
                name, sample, with_negatives, without_negatives))

        print("\n")
if __name__ == '__main__':
    main()

which gives

$ python foo.py 
buggy_trim        ([0, 10, 5])
with_negatives:    [0]
without_negatives: [10, 5]

fixed_trim        ([0, 10, 5])
with_negatives:    []
without_negatives: [0, 10, 5]



buggy_trim        ([1, 2, 5])
with_negatives:    None
without_negatives: [1, 2, 5]

fixed_trim        ([1, 2, 5])
with_negatives:    []
without_negatives: [1, 2, 5]



buggy_trim        ([-1, -2, -5])
with_negatives:    [-1, -2, -5]
without_negatives: []

fixed_trim        ([-1, -2, -5])
with_negatives:    [-1, -2, -5]
without_negatives: None

I no longer seem to have access to this repo so I'll just /cc @c24t @mayurkale22 @songy23

[c24t]

if a bucket boundary starts at 0 e.g. [0, 5, 10, 200], it will always falsely trim the list to [5, 10, 200] but 0 isn't a negative value

That's intentional, we need to drop non-positive boundaries here.

for and then else will ALWAYS run the else clause whenever the for loop terminates so still we falsely report that 1 value is getting dropped

That's also intentional, but the log message is confusing. In the PR that added this line (#389) I changed the message for @mayurkale22's review. I didn't change the code as not to log when we drop a 0-valued boundary because I don't think that's the right behavior: users shouldn't create distributions with 0-valued boundaries, and if they do we ought to log it. See #389 (comment).

[c24t]

I no longer seem to have access to this repo

AFAIK only merge permissions should have changed, you can't assign the issue?