[MRG+1] Instance level common tests #9019

amueller · 2017-06-06T16:46:08Z

This is pulling out another part of #8022: the instance level common tests.
This doesn't give a huge boon by itself, but allows better testing of meta-estimators, because now check_estimators is much more flexible!

Mainly #8022 is still insanely big and I though it might be a good idea to split it up.

amueller · 2017-06-06T16:46:27Z

ping @jnothman @GaelVaroquaux @agramfort @vene ;)

amueller · 2017-06-06T16:47:51Z

sklearn/utils/estimator_checks.py

@@ -1608,16 +1634,16 @@ def param_filter(p):
                assert_equal(param_value, init_param.default)


-def multioutput_estimator_convert_y_2d(name, y):
+def multioutput_estimator_convert_y_2d(estimator, y):


this one is to make the other patch simpler...

…r checks fail)

amueller · 2017-06-06T17:15:25Z

with the tags PR I will simplify test_common a bit and basically just call check_estimator on all estimators, hopefully ;)

vene

(note to self: got up to line ~650)

vene · 2017-06-06T17:08:36Z

sklearn/utils/tests/test_estimator_checks.py

@@ -204,3 +204,4 @@ def __init__(self):
                        check_no_fit_attributes_set_in_init,
                        'estimator_name',
                        NonConformantEstimator)
+    check_estimators_unfitted("estimator", CorrectNotFittedErrorClassifier())


merge error: line 190 duplicated here

vene · 2017-06-06T17:14:46Z

sklearn/utils/estimator_checks.py


    errmsg = "Input contains NaN, infinity or a value too large for " \
             "dtype('float64')."
    try:
-        Estimator().fit(X, y)
+        estimator.fit(X, y)
    except ValueError as e:
        if str(e) != errmsg:
            raise ValueError("Estimator {0} raised warning as expected, but "


(not this PRs fault, but) ambiguous message here: raised warning or error?

vene · 2017-06-06T17:29:48Z

sklearn/tests/test_common.py

-            yield _named_check(check, name), name, Estimator
+        estimator = Estimator()
+        # check this on class
+        yield check_no_fit_attributes_set_in_init, name, Estimator


is this not tested for meta estimators?

Right now the tests for meta-estimators are not very strong. They will be much stronger with the estimator tags branch merged. I tried to keep this PR small, it doesn't increase test coverage much, but it doesn't decrease it anywhere afaik.

coverage is a good sanity check for this indeed. Sounds good.

We should still be using _named_check, no?

If we just reframe check_no_fit_attributes_set_in_init as check_no_fit_attributes_set_upon_clone, perhaps we can get away with running it on an instance too.

vene · 2017-06-06T17:31:52Z

sklearn/utils/estimator_checks.py

@@ -244,11 +253,20 @@ def check_estimator(Estimator):
        Class to check. Estimator is a class object (not an instance).


Could be an instance now :)

vene · 2017-06-06T17:32:58Z

sklearn/utils/estimator_checks.py

@@ -314,7 +332,7 @@ def set_testing_parameters(estimator):
        # of components of the random matrix projection will be probably
        # greater than the number of features.
        # So we impose a smaller number (avoid "auto" mode)
-        estimator.set_params(n_components=1)
+        estimator.set_params(n_components=8)


necessary now that the dictlearning bug is fixed?

Although 1 is very edge-casey here so I'm not opposed to this change even if not strictly necessary. Maybe 2 or so to shave off a few ms

vene · 2017-06-06T17:33:41Z

sklearn/utils/estimator_checks.py

    rng = np.random.RandomState(0)
    X = rng.rand(40, 10)
    X[X < .8] = 0
    X_csr = sparse.csr_matrix(X)
    y = (4 * rng.rand(40)).astype(np.int)
+    y = multioutput_estimator_convert_y_2d(estimator, y)


was the absence of this line a bug?

I think so. Hm also why do we need the scaler special cases down there... huh...

scaling/standardizing when with_mean=True raises an exception rather than densifying the data. I think the decision was that users would ignore warnings and would blow up the memory...

Really the tags for this estimator will be tricky too. Technically with the default args it does not support sparse inputs.

yeah I thought we deprecated that behavior when we introduced MaxAbsScaler but that was as an alternative for MinMaxScaler, not StandardScaler, my bad

vene · 2017-06-06T17:39:07Z

sklearn/utils/estimator_checks.py

        # to not check deprecated classes
        return
    rnd = np.random.RandomState(0)
    X = 3 * rnd.uniform(size=(20, 3))
    y = X[:, 0].astype(np.int)
-    y = multioutput_estimator_convert_y_2d(name, y)
-    estimator = Estimator()


not cloned here

jnothman · 2017-06-07T00:15:16Z

i don't have the time to review this now, but in terms of testing rigour, I'm strongly in support of this concept.

…

On 7 Jun 2017 3:44 am, "Vlad Niculae" ***@***.***> wrote: ***@***.**** commented on this pull request. (note to self: got up to line ~650) ------------------------------ In sklearn/utils/tests/test_estimator_checks.py <#9019 (comment)> : > @@ -204,3 +204,4 @@ def __init__(self): check_no_fit_attributes_set_in_init, 'estimator_name', NonConformantEstimator) + check_estimators_unfitted("estimator", CorrectNotFittedErrorClassifier()) merge error: line 190 duplicated here ------------------------------ In sklearn/utils/estimator_checks.py <#9019 (comment)> : > errmsg = "Input contains NaN, infinity or a value too large for " \ "dtype('float64')." try: - Estimator().fit(X, y) + estimator.fit(X, y) except ValueError as e: if str(e) != errmsg: raise ValueError("Estimator {0} raised warning as expected, but " (not this PRs fault, but) ambiguous message here: raised *warning* or *error*? ------------------------------ In sklearn/tests/test_common.py <#9019 (comment)> : > @@ -63,8 +64,12 @@ def test_non_meta_estimators(): continue if name.startswith("_"): continue - for check in _yield_all_checks(name, Estimator): - yield _named_check(check, name), name, Estimator + estimator = Estimator() + # check this on class + yield check_no_fit_attributes_set_in_init, name, Estimator is this not tested for meta estimators? ------------------------------ In sklearn/utils/estimator_checks.py <#9019 (comment)> : > @@ -244,11 +253,20 @@ def check_estimator(Estimator): Class to check. Estimator is a class object (not an instance). Could be an instance now :) ------------------------------ In sklearn/utils/estimator_checks.py <#9019 (comment)> : > @@ -314,7 +332,7 @@ def set_testing_parameters(estimator): # of components of the random matrix projection will be probably # greater than the number of features. # So we impose a smaller number (avoid "auto" mode) - estimator.set_params(n_components=1) + estimator.set_params(n_components=8) necessary now that the dictlearning bug is fixed? ------------------------------ In sklearn/utils/estimator_checks.py <#9019 (comment)> : > rng = np.random.RandomState(0) X = rng.rand(40, 10) X[X < .8] = 0 X_csr = sparse.csr_matrix(X) y = (4 * rng.rand(40)).astype(np.int) + y = multioutput_estimator_convert_y_2d(estimator, y) was the absence of this line a bug? ------------------------------ In sklearn/utils/estimator_checks.py <#9019 (comment)> : > # to not check deprecated classes return rnd = np.random.RandomState(0) X = 3 * rnd.uniform(size=(20, 3)) y = X[:, 0].astype(np.int) - y = multioutput_estimator_convert_y_2d(name, y) - estimator = Estimator() not cloned here — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9019 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz67ocUgo385CCo-FCaOYX3xSQ-NIEks5sBY_7gaJpZM4Nxndh> .

vene · 2017-06-07T08:33:35Z

sklearn/utils/estimator_checks.py

+            if args[0] == "self":
+                # if_delegate_has_method makes methods into functions
+                # with an explicit "self", so need to shift arguments
+                args = args[1:]


Wow, that was pretty subtle! For reference I made this gist.

What if some clever ass names the first argument something else than self?

It's bad style, and we would probably complain reviewing that code, so I don't think that we should cater for it.

Since this only happens for methods that are passed through @if_delegate_has_method, maybe we could make that decorator raise an error unless its first attribute is called "self", just to avoid subtle breakage in the future?

Open issue?

vene · 2017-06-07T08:50:17Z

Most of this PR consists of changes of the form:

def f(Estimator):

with ignore_warnings(category=DeprecationWarning):
   estimator = Estimator()
   ...

into:

@ignore_warnings(category=DeprecationWarning):
def f(estimator):
    estimator = clone(estimator)
   ...

I checked and we cannot remove the ignore_warnings outright, because cloning a deprecated estimator re-raises the deprecation warning.

However, this has the consequence of possibly ignoring more deprecations, because the coverage is wider. This way we might miss, in the future, if we deprecate e.g. StandardScaler or something and it is used in the common tests.

vene · 2017-06-07T08:52:46Z

sklearn/utils/estimator_checks.py

        set_testing_parameters(classifier)
        # try to fit
        try:
            classifier.fit(X_train, y)
        except ValueError as e:
            if 'class' not in repr(e):
-                print(error_string_fit, Classifier, e)
+                print(error_string_fit, classifier, e)


Should this print classifier.__class__ or similar, or is this ok?

I think that's fine, but I can also change it if you like.

it's fine, printout might be more useful the new way.

vene · 2017-06-07T09:02:15Z

sklearn/utils/estimator_checks.py

@@ -1635,13 +1662,13 @@ def check_non_transformer_estimators_n_iter(name, Estimator):

    # LassoLars stops early for the default alpha=1.0 the iris dataset.
    if name == 'LassoLars':
-        estimator = Estimator(alpha=0.)


(not the fault of this PR but) Just curious why can't we check if n_iter_ is at least 0 instead. LassoLars is doing the right thing: on some datasets there is no work to be done.

feel free to open an issue ;)

vene · 2017-06-07T09:05:20Z

sklearn/utils/estimator_checks.py

-        return
-
-    else:
-        e = estimator()


I assume this logic will move to the caller of this code instead. Nice!

vene · 2017-06-07T09:05:52Z

sklearn/utils/estimator_checks.py

    # Check whether an estimator having both decision_function and
    # predict_proba methods has outputs with perfect rank correlation.

    centers = [(2, 2), (4, 4)]
    X, y = make_blobs(n_samples=100, random_state=0, n_features=4,
                      centers=centers, cluster_std=1.0, shuffle=True)
    X_test = np.random.randn(20, 2) + 4
-    estimator = Estimator()


estimator is not cloned here

amueller · 2017-06-07T09:06:20Z

However, this has the consequence of possibly ignoring more deprecations, because the coverage is wider. This way we might miss, in the future, if we deprecate e.g. StandardScaler or something and it is used in the common tests.

We haven't been consistent with how we ignore warnings in the common tests, and some common test outright ignore all warnings. I think we should not worry about deprecations in the common tests for now.

vene · 2017-06-07T09:07:37Z

re:deprecations, sounds good, and we shouldn't let it slow this PR down.

Other than the couple of missing clones and the few questions I left, this LGTM.

vene · 2017-06-07T09:37:44Z

sklearn/utils/estimator_checks.py

    rng = np.random.RandomState(0)
    X = rng.rand(40, 10)
    X[X < .8] = 0
    X_csr = sparse.csr_matrix(X)
    y = (4 * rng.rand(40)).astype(np.int)
+    y = multioutput_estimator_convert_y_2d(estimator, y)
    for sparse_format in ['csr', 'csc', 'dok', 'lil', 'coo', 'dia', 'bsr']:
        X = X_csr.asformat(sparse_format)
        # catch deprecation warnings
        with ignore_warnings(category=DeprecationWarning):


also here you didn't move the ignore_warnings to the top

yeah, but should be fine, right?

yep just triggered my mindless pattern-matching review process :P

…efore and after cloning.

amueller · 2017-06-07T09:53:02Z

I think I addressed everything. I renamed the check parameters to estimator_org to make sure it's cloned every time.

amueller · 2017-06-07T10:03:57Z

@jnothman this should actually be "easy" to review ;)

vene · 2017-06-07T10:10:34Z

I'm happy now.

jnothman

Is there any difference in runtime due to all the cloning?

You need to add tests to ensure that check_estimator still works on both class and instance.

You should also add a test to check that the object passed into check_estimator is completely unchanged afterwards. This will help to ensure that all current and any future checks include a clone.

I'm sorry if you were hoping for me to give you a green light!

jnothman · 2017-06-07T10:13:06Z

sklearn/tests/test_common.py

-            yield _named_check(check, name), name, Estimator
+        estimator = Estimator()
+        # check this on class
+        yield check_no_fit_attributes_set_in_init, name, Estimator


We should still be using _named_check, no?

jnothman · 2017-06-07T10:20:43Z

sklearn/tests/test_common.py

-            yield _named_check(check, name), name, Estimator
+        estimator = Estimator()
+        # check this on class
+        yield check_no_fit_attributes_set_in_init, name, Estimator


If we just reframe check_no_fit_attributes_set_in_init as check_no_fit_attributes_set_upon_clone, perhaps we can get away with running it on an instance too.

jnothman · 2017-06-07T10:20:52Z

doc/whats_new.rst

@@ -319,6 +319,11 @@ API changes summary
     now only have ``self.estimators_`` available after ``fit``.
     :issue:`7464` by `Lars Buitinck`_ and `Loic Esteve`_.

+   - All checks in ``utils.estimator_checks``, in particular
+     :func:`utils.estimator_checks.check_estimator` now accept estimator
+     instances. Checks other than ``check_estimator`` do not accept


This is currently untrue for check_no_fit_attributes_set_in_init.

jnothman · 2017-06-07T10:24:39Z

sklearn/utils/estimator_checks.py

-    name = Estimator.__name__
-    check_parameters_default_constructible(name, Estimator)
-    for check in _yield_all_checks(name, Estimator):
+    if isinstance(Estimator, type):


I'm not entirely certain this is the right check, but my attempts to break it have failed so far.

~~https://stackoverflow.com/questions/395735/how-to-check-whether-a-variable-is-a-class-or-not~~ sorry, should have looked at the date...

According to my knowledge and to a quick Google, this is the right way to do it.

jnothman · 2017-06-07T10:29:42Z

sklearn/utils/estimator_checks.py

-def _yield_non_meta_checks(name, Estimator):
+def assert_almost_equal_dense_sparse(x, y, decimal=6, err_msg=''):
+    if sparse.issparse(x):
+        assert_array_almost_equal(x.data, y.data,


(fwiw, assert_allclose may be preferable...)

name's shorter? (i.e. not obscenely verbose) :D

More seriously, because it allows control in terms of absolute and relative tolerance, which allows you to compare very large numbers, very small numbers, etc., while decimals is irrelevant to the internal number format.

TBH, I've seen it said somewhere. And I can't recall the arguments put forth, but I like the brief name, and I like that we're able to use rtol when appropriate, and it's good practice to be thinking about whether absolute or relative tolerance is the issue for a particular case.

jnothman · 2017-06-07T10:41:37Z

sklearn/utils/estimator_checks.py

@@ -314,7 +336,7 @@ def set_testing_parameters(estimator):
        # of components of the random matrix projection will be probably
        # greater than the number of features.
        # So we impose a smaller number (avoid "auto" mode)
-        estimator.set_params(n_components=1)
+        estimator.set_params(n_components=2)


set_testing_params becomes problematic now. We will potentially hide bugs by overriding explicit tests of parameter settings. We should only override these if they are currently at their default values.

Thanks for incidentally changing something in this function so that I was reminded that it was an issue!

why does it become problematic now, wasn't it problematic to begin with?

No, because before we were changing defaults from what was most useful for a user to what is most practical in testing. Now if a developer explicitly passes a non-default instantiation, we're ignoring their wishes.

Do you have a suggestion how to resolve that? I hadn't thought about that but agree

great point. only override a param if it is ~~different~~ not different from the default?

Yes, putting this in test_classifiers or whatever might make sense. Hopefully we don't break too many downstream libraries using check_estimator this way.

or we use an ugly hack, like checking if the estimator comes from sklearn ;) but that wouldn't allow us to specify parameters safely.

that requires adding a use_fast_parameters to all of the checks, right?
Alternatively, we could get rid of the function, and have each estimator declare its testing parameters.

no wait, I got it...

What is the conclusion of this discussion (https://github.com/scikit-learn/scikit-learn/pull/9019/files#r120591375)?

jnothman · 2017-06-07T10:43:14Z

sklearn/utils/estimator_checks.py

@@ -337,20 +359,24 @@ def _is_32bit():
    return struct.calcsize('P') * 8 == 32


-def check_estimator_sparse_data(name, Estimator):
+def check_estimator_sparse_data(name, estimator_org):


As someone who has worked too much on Named Entity Recognition, I can't read "org" as anything but "organisation". Can we use estimator_orig or base_estimator?

base_estimator means something different to me. I orig is fine for me

+1 for orig

jnothman · 2017-06-07T10:54:45Z

sklearn/utils/estimator_checks.py



-def check_estimators_partial_fit_n_features(name, Alg):
+@ignore_warnings(category=DeprecationWarning)
+def check_estimators_partial_fit_n_features(name, alg_org):


rename alg to estimator?

jnothman · 2017-06-07T10:56:39Z

sklearn/utils/estimator_checks.py

 def check_no_fit_attributes_set_in_init(name, Estimator):
    """Check that Estimator.__init__ doesn't set trailing-_ attributes."""
+    # STILL ON CLASSES


Once the world of scikit-learn <= 0.18 is forgotten, this comment will look pretty obscure. How about "Takes a class rather than an estimator instance as input"

Makes sense

also it's nice to have fewer comments that yell at the reader 😛

jnothman · 2017-06-07T10:57:19Z

sklearn/utils/estimator_checks.py

@@ -1547,6 +1584,7 @@ def check_estimators_data_not_an_array(name, Estimator, X, y):


 def check_parameters_default_constructible(name, Estimator):
+    # THIS ONE IS STILL ON CLASSES


Should we be doing type(estimator)() instead?

Well, but I want to support non-default constructible estimators in check_estimator, like GridSearchCV.

Okay, I guess. At some point in the future, I think we will want a way to specify testing parameters for non-default-constructables.

amueller · 2017-06-07T11:11:52Z

@jnothman I was hoping for you to nag as much as possible ;) there are some tests on instances and some on classes but I should make that more explicit.

amueller · 2017-06-09T10:57:42Z

should be good now.

GaelVaroquaux · 2017-06-09T11:07:03Z

LGTM. +1 for merge.

Does anybody else wants to comment, or should we merge?

vene · 2017-06-09T11:30:10Z

I should have either tested locally on old scipy, or you should set up CI for prs onto your fork 😛

in my defence I did check with modern scipy.

vene · 2017-06-09T11:35:33Z

sklearn/utils/estimator_checks.py

-    e = Estimator()
-    set_testing_parameters(e)
+def check_estimators_empty_data_messages(name, estimator_orig):
+    e = clone(estimator_orig)


e -> estimator?

meh, I just saw that there is also est all over the place. Nevermind.

vene · 2017-06-09T11:49:46Z

genuine travis failure on py27 ubuntu; RandomForest fails the pickle test it seems

fix flake8 error and shorten

amueller · 2017-06-09T12:37:07Z

can't reproduce :-/

amueller · 2017-06-09T13:01:45Z

is what's happening on travis (top before, bottom after)

… is weird

…rn into instance_level_tests

ogrisel

LGTM

ogrisel · 2017-06-09T13:56:51Z

sklearn/utils/estimator_checks.py

@@ -314,7 +336,7 @@ def set_testing_parameters(estimator):
        # of components of the random matrix projection will be probably
        # greater than the number of features.
        # So we impose a smaller number (avoid "auto" mode)
-        estimator.set_params(n_components=1)
+        estimator.set_params(n_components=2)


What is the conclusion of this discussion (https://github.com/scikit-learn/scikit-learn/pull/9019/files#r120591375)?

ogrisel · 2017-06-09T14:03:53Z

sklearn/utils/estimator_checks.py

@@ -870,23 +883,22 @@ def check_estimators_nan_inf(name, Estimator):
    for X_train in [X_train_nan, X_train_inf]:
        # catch deprecation warnings
        with ignore_warnings(category=DeprecationWarning):


This is now caught at the check function level.

jnothman · 2017-06-10T14:22:48Z

Congrats, @amueller. I suppose we should start testing some metaestimators etc We maybe should update the advice to contrib devs too.

…

On 10 Jun 2017 12:34 am, "Andreas Mueller" ***@***.***> wrote: Merged #9019 <#9019>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9019 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6_HKrxw3j4y3kMbDNMUQmxsSkzpyks5sCVfYgaJpZM4Nxndh> .

amueller · 2017-06-10T14:38:53Z

@jnothman thanks and yeah, for sure. I'd like to get #8022 in, so we can actually give contrib devs useful advice. Right now they still might not be able to test estimators.

amueller added 2 commits June 6, 2017 18:30

start work on separating instance-level tests

8666465

minor refactoring / fixes to work without tags

bbfaf59

amueller commented Jun 6, 2017

View reviewed changes

amueller added 3 commits June 6, 2017 19:12

add clone into check_supervised_y_2d estimator check (which made othe…

4dd732d

…r checks fail)

remove duplicate check_estimator_unfitted assert

7ce1123

add issue reference to whatsnew entry

48bd931

amueller changed the title ~~Instance level common tests~~ [MRG] Instance level common tests Jun 6, 2017

vene reviewed Jun 6, 2017

View reviewed changes

vene reviewed Jun 7, 2017

View reviewed changes

sklearn/utils/estimator_checks.py

return

else:

e = estimator()

Copy link

Member

vene Jun 7, 2017

Choose a reason for hiding this comment

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

I assume this logic will move to the caller of this code instead. Nice!

vene reviewed Jun 7, 2017

View reviewed changes

amueller added 2 commits June 7, 2017 11:47

added some clones, minor fixes from vene's review

b1171ed

rename estimator arg to estimator_org to make a visible distinction b…

c636b20

…efore and after cloning.

amueller force-pushed the instance_level_tests branch from 79bd264 to c636b20 Compare June 7, 2017 09:48

more renaming for more explicit clones

7eb6bed

vene changed the title ~~[MRG] Instance level common tests~~ [MRG+1] Instance level common tests Jun 7, 2017

jnothman reviewed Jun 7, 2017

View reviewed changes

vene reviewed Jun 9, 2017

View reviewed changes

fix pep8 and shorten

cce8954

vene mentioned this pull request Jun 9, 2017

[MRG+2] TransformedTargetRegressor #9041

Merged

3 tasks

Merge pull request #32 from vene/flakefix

ef97a81

fix flake8 error and shorten

amueller added 2 commits June 9, 2017 15:29

use joblib.hash for inequality check because the pickle state machine…

46189b8

… is weird

Merge branch 'instance_level_tests' of github.com:amueller/scikit-lea…

b151752

…rn into instance_level_tests

ogrisel approved these changes Jun 9, 2017

View reviewed changes

amueller merged commit d788dcb into scikit-learn:master Jun 9, 2017

This was referenced Jun 10, 2017

MetaEstimators hard-coded in common tests. #6079

Closed

allow testing meta_estimators from outside scikit-learn #6423

Closed

Testing pipeline with check_estimator #6594

Closed

Ability to specify parameters for common tests #7289

Closed

massich mentioned this pull request Nov 7, 2018

[FIX] Repair scaling of data in _compute_covariance_auto mne-tools/mne-python#5698

Merged

thomasjpfan mentioned this pull request Apr 24, 2020

Remove support for classes in check_estimator and parametrize_with_checks #17030

Closed

		@@ -244,11 +253,20 @@ def check_estimator(Estimator):
		Class to check. Estimator is a class object (not an instance).

		@@ -1547,6 +1584,7 @@ def check_estimators_data_not_an_array(name, Estimator, X, y):


		def check_parameters_default_constructible(name, Estimator):
		# THIS ONE IS STILL ON CLASSES

[MRG+1] Instance level common tests #9019

[MRG+1] Instance level common tests #9019

Conversation

amueller commented Jun 6, 2017

amueller commented Jun 6, 2017

Choose a reason for hiding this comment

amueller commented Jun 6, 2017

vene 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

Choose a reason for hiding this comment

jnothman commented Jun 7, 2017 via email

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

vene commented Jun 7, 2017

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

amueller commented Jun 7, 2017

vene commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Jun 7, 2017

amueller commented Jun 7, 2017

vene commented Jun 7, 2017

jnothman 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

vene Jun 8, 2017 • edited Loading

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

vene Jun 7, 2017 • edited Loading

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

ogrisel Jun 9, 2017 • edited Loading

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

vene Jun 8, 2017 •

edited

Loading

vene Jun 7, 2017 •

edited

Loading

ogrisel Jun 9, 2017 •

edited

Loading