Yelp / MOE

A global, black box optimization engine for real world metric optimization.
Other
1.31k stars 139 forks source link

Fixed str and add bugs in bandit data containers #389

Closed norases closed 10 years ago

norases commented 10 years ago

***** PEOPLE ***** Primary reviewer: @sc932

Reviewers: @suntzu86 @Feriority

***** DESCRIPTION ** Branch Name: norases_387_fix_bandit_bugs_and_add_new_tests Ticket(s)/Issue(s): Closes #387

***** TESTING DONE ***** make test make style-test

suntzu86 commented 10 years ago

might want one more test, o/w looks good!

sc932 commented 10 years ago

Yeah, let's add a test for add :)

sc932 commented 10 years ago

You rock! :rocket: Ship it!

suntzu86 commented 10 years ago

2 potential things:

  1. add is fine for instance + instance and instance + noninstance. If you want to support noninstance + instance, you need __radd__ which can be implemented in terms of add: https://www.inkling.com/read/learning-python-mark-lutz-4th/chapter-29/right-side-and-in-place
  2. You should implement __iadd__. iadd is for +=. add and iadd will be very similar; just iadd will return self. It works right now b/c it's turning "x += y" into "x = x + y" which may be surprising to users who will expect += on mutable objects to operate in-place.

im still checking around to see what's idiomatic

suntzu86 commented 10 years ago

Updates after talking to @Feriority:

  1. set __radd__ = __add__ since addition is commutative.
  2. implement `iadd``. a testcase from @Feriority indicating why the current behavior would be shocking:

    >>> def add_lists(x, y):
    ...     x+=y
    ...
    >>> a = [1, 2, 3]
    >>> b = [4, 5, 6]
    >>> add_lists(a, b)
    >>> a
    [1, 2, 3, 4, 5, 6]
    >>>

    Without a iadd function, the last print of a would give its initial value.

  3. probably worth calling validate() again after addition just to make sure someone didn't set wins = -10000 like you pointed out.
suntzu86 commented 10 years ago

adding @Feriority as a reviewer; his input has been very handy in helping me understand this python stuff :)

suntzu86 commented 10 years ago

lastly, I'm not 100% sure what the easiest way to test iadd is. I think checking the object id. Like:

x = ...
y = ...
x_old_ix = id(x)
x += y
x_new_id = id(x)
assert_equal(x_old_id, x_new_id)

ids would only match if x is not a new object (which only happens if iadd is implemented). I think this is robust & safe

suntzu86 commented 10 years ago

couple of comment changes and one additional thing to check in testing o/w looks good to me

Feriority commented 10 years ago

Mostly looks good. Only comment is the docstring lines are pretty long, and should probably be wrapped to multiple lines. Fix & ship.

norases commented 10 years ago

I fixed a comment line that is really long. the rest @sc932 said it is ok. Also broke up the assertion in last test