[MRG+1] Issue#7998 : Consistent parameters between QDA and LDA #8130

mrbeann · 2016-12-28T16:58:56Z

Reference Issue

Fix #7998

see also the previous one

mrbeann · 2016-12-29T10:47:54Z

Thanks @johannah . I have made a new PR according to your guidance.
#8134

PS:You mentioned:

You've still got a number of other peoples' commits showing in your diff :\

But I'm not really sure, In this PR, there just one commit

jnothman · 2016-12-29T10:49:32Z

I think this one is looking okay. Please just be sure to close what is not current. Then if you can avoid opening new PRs, that would be great.

mrbeann · 2016-12-29T10:52:08Z

Thank you, I will close them. Sorry for the inconveniences.

jnothman · 2016-12-29T10:57:57Z

Also, in general you can set the PR to any commit / branch / history you like. In your PR branch, just use: git reset --hard some_commit_ref then git push --force # add whatever args you need to push to your fork

…

On 29 December 2016 at 21:52, JC Liu ***@***.***> wrote: Thank you, I will close them. Sorry for the inconveniences. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8130 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz68jIRWwlxNuJ6HrS0EPHx_BvI0nbks5rM5DagaJpZM4LXDPN> .

mrbeann · 2016-12-29T11:03:13Z

Okay, learned a lot from this process, thank you for your help.
And to my understanding #8130 and #8134 are both okay, so I can close one of them? then make the future change in the remaining one?

jnothman · 2016-12-29T11:23:48Z

yes

…

On 29 December 2016 at 22:03, JC Liu ***@***.***> wrote: Okay, learned a lot from this process, thank you for your help. And to my understanding #8130 <#8130> and #8134 <#8134> are both okay, so I can close one of them? then make the future change in the remaining one? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8130 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6zrpWUK1fqj3zsO7Lr0KRTyq81tVks5rM5NzgaJpZM4LXDPN> .

mrbeann · 2016-12-29T12:07:25Z

Okay, just close #8134

jnothman · 2016-12-29T12:59:11Z

sklearn/discriminant_analysis.py

@@ -130,6 +130,7 @@ def _class_cov(X, y, priors=None, shrinkage=None):

 class LinearDiscriminantAnalysis(BaseEstimator, LinearClassifierMixin,
                                 TransformerMixin):
+


please don't insert new lines into untouched parts of the code

jnothman · 2017-01-02T02:35:38Z

sklearn/discriminant_analysis.py

@@ -289,6 +291,9 @@ def _solve_lsqr(self, X, y, shrinkage):
           0-471-05669-3.
        """
        self.means_ = _class_means(X, y)
+        if not self.store_covariance:
+            warnings.warn("'covariance_' will be stored only if "
+                          "'store_covariance' is True", DeprecationWarning)


add "from version 0.21"

jnothman · 2017-01-02T02:35:42Z

sklearn/discriminant_analysis.py

@@ -327,6 +332,9 @@ class scatter). This solver supports both classification and
           0-471-05669-3.
        """
        self.means_ = _class_means(X, y)
+        if not self.store_covariance:
+            warnings.warn("'covariance_' will be stored only if "
+                          "'store_covariance' is True", DeprecationWarning)


add "from version 0.21"

jnothman · 2017-01-02T02:35:53Z

sklearn/discriminant_analysis.py

@@ -534,6 +542,7 @@ def predict_log_proba(self, X):


 class QuadraticDiscriminantAnalysis(BaseEstimator, ClassifierMixin):
+


please remove blank line

jnothman · 2017-01-02T02:36:33Z

sklearn/discriminant_analysis.py

-        if self.store_covariances:
+        if self.store_covariances is not None:
+            warnings.warn("'store_covariances' was renamed to store_covariance"
+                          " in version 0.19 and will be removed in 0.20.",


jnothman · 2017-01-02T02:37:21Z

sklearn/discriminant_analysis.py

+            warnings.warn("'store_covariances' was renamed to store_covariance"
+                          " in version 0.19 and will be removed in 0.20.",
+                          DeprecationWarning)
+            self.store_covariance = self.store_covariances


Better to use a local variable so that we don't modify the already set parameter.

jnothman · 2017-01-02T02:40:13Z

sklearn/tests/test_discriminant_analysis.py

+def test_lda_store_covariance():
+    # The default is to not set the covariances_ attribute
+    clf = LinearDiscriminantAnalysis().fit(X, y)
+    assert_true(not hasattr(clf, 'covariance_'))


perhaps assert_false!

jnothman · 2017-01-02T02:41:22Z

sklearn/tests/test_discriminant_analysis.py

@@ -223,6 +223,26 @@ def test_lda_scaling():
                     'using covariance: %s' % solver)


+def test_lda_store_covariance():
+    # The default is to not set the covariances_ attribute
+    clf = LinearDiscriminantAnalysis().fit(X, y)


Please test with each solver available.

jnothman · 2017-01-02T02:42:31Z

sklearn/tests/test_discriminant_analysis.py

        np.array([[0.7, 0.45], [0.45, 0.7]])
    )

    assert_array_almost_equal(
-        clf.covariances_[1],
+        clf.covariance_[1],
        np.array([[0.33333333, -0.33333333], [-0.33333333, 0.66666667]])
    )


Please also test the deprecation using assert_warns

mrbeann · 2017-01-03T06:34:14Z

@jnothman Just fix these, please check them

jnothman

I don't mind whether or not you do it here, but it can be useful to test deprecated functionality separately from testing continuing behaviour. This makes it easier to remove the relevant tests when the deprecation concludes.

Thanks!

jnothman · 2017-01-04T09:16:19Z

sklearn/tests/test_discriminant_analysis.py

+    # Test for slover 'lsqr' and 'eigen'
+    for solver in ('lsqr', 'eigen'):
+        clf = LinearDiscriminantAnalysis(solver=solver)
+        assert_false(not hasattr(clf.fit(X, y), 'covariance_'))


this is assert_true(hasattr...), no?

jnothman · 2017-01-04T09:16:40Z

sklearn/discriminant_analysis.py

-               *store_covariance* has been moved to main constructor.
+
+               *store_covariances* has been moved to the main constructor as
+                *store_covaraiance*


please also remove the blank line before.

jnothman · 2017-01-04T09:17:50Z

sklearn/discriminant_analysis.py

@@ -556,9 +563,15 @@ class QuadraticDiscriminantAnalysis(BaseEstimator, ClassifierMixin):
        Regularizes the covariance estimate as
        ``(1-reg_param)*Sigma + reg_param*np.eye(n_features)``

+    store_covariance : boolean
+        If True the covariance matrices are computed and stored in the
+        `self.covariance_` attribute.


use double-backticks for code formatting and you can drop self

jnothman · 2017-01-04T09:18:46Z

sklearn/discriminant_analysis.py

@@ -289,6 +290,9 @@ def _solve_lsqr(self, X, y, shrinkage):
           0-471-05669-3.
        """
        self.means_ = _class_means(X, y)
+        if not self.store_covariance:
+            warnings.warn("from version 0.21 'covariance_' will be stored only"


I think this should only be raised when covariance_ is accessed and present by error. I.e. do it with a property.

Hi @jnothman Thanks for your review. But I'm not sure what this mean here. Can you give me more detail or examples?
Thank you!

I mean you will need something like:

@property def covariance_(self): if hasattr(self, '_covariance_') and self.store_covariance == False: warnings.warn ... return self._covariance_

It's a bit ugly, but still.

jnothman · 2017-01-04T09:19:57Z

sklearn/tests/test_discriminant_analysis.py

+        assert_false(not hasattr(clf.fit(X, y), 'covariance_'))
+
+        # Test the deprecation
+        assert_warns(DeprecationWarning, clf.fit, X, y)


Would be better to use assert_warns_message to clarify which deprecation this is.

jnothman · 2017-01-04T09:20:38Z

sklearn/tests/test_discriminant_analysis.py


    # Test the actual attribute:
-    clf = QuadraticDiscriminantAnalysis(store_covariances=True).fit(X6, y6)


Ideally, we should check that store_covariances still works but raises a deprecation warning.

mrbeann · 2017-01-07T08:04:45Z

Hi @jnothman , please check if I'm in the right direction.
Thanks

jnothman

Thanks!

jnothman · 2017-01-07T13:08:21Z

sklearn/discriminant_analysis.py

@@ -188,7 +188,7 @@ class LinearDiscriminantAnalysis(BaseEstimator, LinearClassifierMixin,
    intercept_ : array, shape (n_features,)
        Intercept term.

-    covariance_ : array-like, shape (n_features, n_features)
+    _covariance_ : array-like, shape (n_features, n_features)


the public attribute is still covariance_

jnothman · 2017-01-07T13:08:42Z

sklearn/discriminant_analysis.py

+    @property
+    def covariance_(self):
+        if hasattr(self, '_covariance_') and not self.store_covariance:
+            warnings.warn("from version 0.21 '_covariance_' will be stored "


_covariance_ -> covariance_

jnothman · 2017-01-07T13:08:47Z

sklearn/discriminant_analysis.py

+    @covariance_.setter
+    def covariance_(self, value):
+        if hasattr(self, '_covariance_') and not self.store_covariance:
+            warnings.warn("from version 0.21 '_covariance_' will be stored "


_covariance_ -> covariance_

jnothman · 2017-01-07T13:09:31Z

sklearn/discriminant_analysis.py

@@ -599,7 +616,7 @@ class QuadraticDiscriminantAnalysis(BaseEstimator, ClassifierMixin):
    >>> clf.fit(X, y)
    ... # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE
    QuadraticDiscriminantAnalysis(priors=None, reg_param=0.0,
-                                  store_covariances=False, tol=0.0001)
+                   store_covariance=False, store_covariances=None, tol=0.0001)


PEP8 visual indentation would have this line aligned with the opening parenthesis

jnothman · 2017-01-07T13:09:59Z

sklearn/discriminant_analysis.py

+    @property
+    @deprecated("Attribute covariances_ was deprecated in version"
+                " 0.19 and will be removed in 0.21. Use "
+                "covariance_' instead")


jnothman · 2017-01-07T13:11:31Z

sklearn/tests/test_discriminant_analysis.py

+        )
+
+        assert_array_almost_equal(
+            clf._covariance_[1],


_covariance_ -> covariance_

jnothman · 2017-01-07T13:13:36Z

sklearn/tests/test_discriminant_analysis.py

+
+    # Test for slover svd, the default is to not set the covariances_ attribute
+    clf = LinearDiscriminantAnalysis(solver='svd').fit(X, y)
+    assert_true(not hasattr(clf, '_covariance_'))


_covariance_ -> covariance_

assert_false(hasattr

jnothman · 2017-01-07T13:13:40Z

sklearn/tests/test_discriminant_analysis.py

+    # Test the actual attribute:
+    clf = LinearDiscriminantAnalysis(solver=solver,
+                                     store_covariance=True).fit(X6, y6)
+    assert_true(hasattr(clf, '_covariance_'))


_covariance_ -> covariance_

jnothman · 2017-01-07T13:14:25Z

sklearn/tests/test_discriminant_analysis.py

+        assert_warns_message(DeprecationWarning, "from version 0.21 "
+                             "'_covariance_' will be stored only if "
+                             "'store_covariance' is True",
+                             clf.fit, X, y)


I think this warning should only trigger on getattr(clf.fit(X, y), 'covariance_')

Thanks for your review! I'm not sure is this means I need a deprecation in __getattr__ instead of now with property?
To my understanding we need covariance_ as a "private value" but not sure which way to achieve this?

No, the covariance_ attribute is public. It may present a public face to a __dict__ entry, _covariance_, and do so by being implemented as a descriptor with a getter and setter. The getter is called whenever hasattr(obj, 'covariance_') or getattr(obj, 'covariance_') is called on an instance of LinearDiscriminantAnalysis. If the getter raises AttribtueError, hasattr returns False; otherwise True. getattr returns the result of the getter and propagates the exception.

jnothman · 2017-01-07T13:15:02Z

sklearn/tests/test_discriminant_analysis.py

+    # assert_warns(DeprecationWarning, clf.fit, X, y)
+    assert_warns_message(DeprecationWarning, "'store_covariances' was renamed"
+                         " to store_covariance in version 0.19 and will be "
+                         "removed in 0.21.", clf.fit, X, y)


Also check that covariance_ (and covariances_ with warning) is stored.

jnothman · 2017-01-10T06:17:41Z

sklearn/discriminant_analysis.py

@@ -255,6 +256,36 @@ def __init__(self, solver='svd', shrinkage=None, priors=None,
        self.store_covariance = store_covariance  # used only in svd solver
        self.tol = tol  # used only in svd solver

+    def __get__(self, obj, objtype):


I don't think you mean this. defining __get__ turns the class into a descriptor...

mrbeann · 2017-01-10T13:26:11Z

Hi @jnothman , I misunderstood this, and I have just fixed it, please help me checking if I am in the right direction. Thanks!

mrbeann · 2017-03-03T05:44:16Z

Hi, @jnothman , I think I'm stuck here, can you offer some more guidance? Thank you

jnothman

Otherwise LGTM

jnothman · 2017-03-05T22:41:09Z

sklearn/tests/test_discriminant_analysis.py

+    # Test for slover 'lsqr' and 'eigen'
+    for solver in ('lsqr', 'eigen'):
+        clf = LinearDiscriminantAnalysis(solver=solver)
+        assert_true(hasattr(clf.fit(X, y), 'covariance_'))


Please use assert_warns on the hasattr to check that this behaviour is deprecated appropriately.

Either do that here and remove the test below, or make this test only about non-deprecated behaviour and avoid this assertion with a comment saying that the opposite should be asserted once the deprecation is complete.

jnothman · 2017-03-05T22:41:12Z

sklearn/tests/test_discriminant_analysis.py

+    # check that covariance_ (and covariances_ with warning) is stored
+    clf = QuadraticDiscriminantAnalysis(store_covariances=True).fit(X6, y6)
+    assert_true(hasattr(clf, 'covariance_'))
+    assert_true(hasattr(clf, 'covariances_'))


We do not want the tests issuing warnings. Drop this line and replace hasattr with getattr in the next statement to test the same thing but also catch the warning.

ogrisel · 2017-07-24T14:31:52Z

sklearn/tests/test_discriminant_analysis.py

+            np.array([0.088889, 0.533333])
+        )
+
+    # Test for slover svd, the default is to not set the covariances_ attribute


test with the SVD solver

ogrisel · 2017-07-24T14:33:09Z

sklearn/tests/test_discriminant_analysis.py

+        )
+
+    # Test for slover svd, the default is to not set the covariances_ attribute
+    clf = LinearDiscriminantAnalysis(solver='svd').fit(X, y)


Why use X, y instead of X6, y6 as in the other checks?

Thanks! Actually, there's no special purpose, I just change it to X, y

lesteve · 2017-07-24T16:32:43Z

doc/whats_new.rst

+     :issue:`8174` by :user:`Tahar Zanouda <tzano>`, `Alexandre Gramfort`_
+     and `Raghav RV`_.
+
+   - SciPy >= 0.13.3 and NumPy >= 1.8.2 are now the minimum supported versions


It looks like there is some unwanted changes in this diff. The only thing that should be added is your paragraph about store_covariances_ and covariances_

Sorry, these seem caused by merge conflicts. I just fix it. Thanks!

amueller · 2017-07-24T17:44:44Z

sklearn/discriminant_analysis.py

@@ -245,6 +245,7 @@ class LinearDiscriminantAnalysis(BaseEstimator, LinearClassifierMixin,
    >>> print(clf.predict([[-0.8, -1]]))
    [1]
    """
+


Looks like the docstring doesn't explain that store_covariance only makes a difference if solver='svd'

I think it did last I looked

huh either it changed or I'm blind...

amueller

Looks good apart from minor nitpicks.

amueller · 2017-07-24T17:45:31Z

sklearn/tests/test_discriminant_analysis.py

+def test_lda_store_covariance():
+    # Test for slover 'lsqr' and 'eigen'
+    for solver in ('lsqr', 'eigen'):
+        clf = LinearDiscriminantAnalysis(solver=solver).fit(X6, y6)


Maybe add a comment that store_covariance has no effect on these solvers?

amueller · 2017-07-24T17:47:18Z

sklearn/tests/test_discriminant_analysis.py

+
+    # check that covariance_ (and covariances_ with warning) is stored
+    clf = QuadraticDiscriminantAnalysis(store_covariances=True).fit(X6, y6)
+    assert_true(hasattr(clf, 'covariance_'))


I would remove this (because it creates a DeprecationWarning) and store the result of assert_warns_message and ensure that it's True.

This line should not raise a warning. The fit on the line before must. It should be changed to assert or ignore the warning

lesteve

Some comments

lesteve · 2017-07-25T06:31:26Z

sklearn/discriminant_analysis.py

+            warnings.warn("'store_covariances' was renamed to store_covariance"
+                          " in version 0.19 and will be removed in 0.21.",
+                          DeprecationWarning)
+            store_covariance = self.store_covariances


To simplify the code, I think we should have:

store_covariance = self.store_covariance or self.store_covariances if store_covariance: # some code

but we can't figure out the value of store_covariance is come from self.store_covariance or self.store_covariances, then we can't decide whether to raise a warning?

The only thing you need for the warning is self.store_covariances.

lesteve · 2017-07-25T06:34:56Z

sklearn/tests/test_discriminant_analysis.py

+
+        assert_array_almost_equal(
+            clf.covariance_[0],
+            np.array([0.422222, 0.088889])


Where do the expected covariance come from? Is there a way to compute them in a different way in order to make the test less magical?

Not familiar with this part, you mean we should find another program or in sklearn we have another choice?

Any idea about this? @jnothman Thanks!

As long as the covariance computed by each solver is asserted equal, I think that's more than adequate. They are, after all, used within these techniques to produce the overall model.

Why is this split into two asserts? It's a single covariance matrix, right?

lesteve · 2017-07-25T06:38:38Z

sklearn/tests/test_discriminant_analysis.py

+                         "removed in 0.21.", clf.fit, X, y)
+
+    # check that covariance_ (and covariances_ with warning) is stored
+    clf = QuadraticDiscriminantAnalysis(store_covariances=True).fit(X6, y6)


You don't need to redefine clf here I think and can just reuse the existing clf

lesteve · 2017-07-25T06:43:15Z

sklearn/discriminant_analysis.py

    def fit(self, X, y):
        """Fit the model according to the given training data and parameters.

            .. versionchanged:: 0.19
-               *store_covariance* has been moved to main constructor.
+               *store_covariances* has been moved to main constructor as


Improvement: store_covariances is not a parameter of fit anymore. Use the store_covariance parameter in the constructor instead.

Also I would use double-backticks (monospace in reST) for the parameter names rather than asterisk (italic in reST).

Not sure what this means, we already have store_covariances in the constructor. should we add store_covariance ?

It means your formulation is not clear to me and I would use the one I proposed instead.

amueller · 2017-07-27T16:18:18Z

sklearn/tests/test_discriminant_analysis.py

+                                     store_covariance=True).fit(X6, y6)
+    assert_true(hasattr(clf, 'covariance_'))
+
+    assert_array_almost_equal(


why is this split into two asserts?

amueller · 2017-07-31T16:11:48Z

sklearn/discriminant_analysis.py

        self.tol = tol

+    @property
+    @deprecated("Attribute covariances_ was deprecated in version"
+                " 0.19 and will be removed in 0.21. Use "


Deprecated in 0.20 and removed in 0.22?

@jnothman How do you think about this?

jnothman · 2017-08-01T07:36:39Z

I'd still be okay with merging this in 0.19 final. it's not controversial.

…

On 1 Aug 2017 3:48 am, "JC Liu" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sklearn/discriminant_analysis.py <#8130 (comment)> : > self.tol = tol + @Property + @deprecated("Attribute covariances_ was deprecated in version" + " 0.19 and will be removed in 0.21. Use " @jnothman <https://github.com/jnothman> How do you think about this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8130 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz64PWtyqeg9QYWd9gsGRQ2hc6mN3cks5sThNlgaJpZM4LXDPN> .

amueller · 2017-08-01T20:42:28Z

ok cool.

mrbeann · 2017-08-01T23:45:28Z

Thank you for your guidance! @jnothman @amueller @agramfort @ogrisel

jnothman · 2017-08-01T23:52:57Z

Thanks for your work. Sorry about leading you up the garden path! I'm glad this is merged.

…

On 2 August 2017 at 09:45, JC Liu ***@***.***> wrote: Thank you for your guidance! @jnothman <https://github.com/jnothman> @amueller <https://github.com/amueller> @agramfort <https://github.com/agramfort> @ogrisel <https://github.com/ogrisel> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8130 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6xP5m8oHQqiNPqe782M7oqBjYpyNks5sT7iagaJpZM4LXDPN> .

…t-learn#8130) * for scikit-learn#7998 * Fix some style error and add test * Add local variable store_covariance * better deprecation * fix bug * Style check * fix covariance_ * style check * Update * modify test * Formating * update * Update * Add whats_new.rst * Revert "Add whats_new.rst" This reverts commit 4e5977d. * whats_new * Update for FutureWarning * Remove warning from the setter * add fit in test * drop back * Quick fix * Small fix * Fix * update new * Fix space * Fix docstring * fix style * Fix * fix assert

for scikit-learn#7998

eaef154

This was referenced Dec 29, 2016

[MRG] Issue#7998 : Consistent parameters between QDA and LDA #8118

Closed

[MRG] rename store_covariances to store_covariance #8133

Closed

jnothman reviewed Jan 2, 2017

View reviewed changes

JC Liu added 2 commits January 3, 2017 11:30

Fix some style error and add test

96b3538

Add local variable store_covariance

adbdb0e

jnothman reviewed Jan 4, 2017

View reviewed changes

liu120525 and others added 3 commits January 7, 2017 13:14

better deprecation

ca13587

fix bug

6dbf748

Style check

851a0bf

jnothman reviewed Jan 7, 2017

View reviewed changes

mrbeann added 2 commits January 10, 2017 13:02

fix covariance_

40a42da

style check

3fa211d

jnothman reviewed Jan 10, 2017

View reviewed changes

Update

334cb6a

Neurrone mentioned this pull request Feb 18, 2017

Consistent parameters between QDA and LDA #7998

Closed

jnothman reviewed Mar 5, 2017

View reviewed changes

jnothman changed the title ~~[MRG] Issue#7998 : Consistent parameters between QDA and LDA~~ [MRG+1] Issue#7998 : Consistent parameters between QDA and LDA Mar 5, 2017

modify test

ecba66a

Small fix

6017e26

ogrisel reviewed Jul 24, 2017

View reviewed changes

Fix

0b47d5c

lesteve reviewed Jul 24, 2017

View reviewed changes

mrbeann added 2 commits July 25, 2017 00:59

update new

1a1fa87

Fix space

f3fcd53

amueller reviewed Jul 24, 2017

View reviewed changes

mrbeann added 2 commits July 25, 2017 10:41

Fix docstring

162d90a

fix style

c187197

lesteve reviewed Jul 25, 2017

View reviewed changes

Fix

a7c2cd0

amueller reviewed Jul 27, 2017

View reviewed changes

fix assert

9abe7df

amueller reviewed Jul 31, 2017

View reviewed changes

amueller merged commit 72d0529 into scikit-learn:master Aug 1, 2017

mrbeann deleted the pr7998 branch August 1, 2017 23:45

mrbeann restored the pr7998 branch August 1, 2017 23:45

		@@ -130,6 +130,7 @@ def _class_cov(X, y, priors=None, shrinkage=None):

		class LinearDiscriminantAnalysis(BaseEstimator, LinearClassifierMixin,
		TransformerMixin):

		@@ -534,6 +542,7 @@ def predict_log_proba(self, X):


		class QuadraticDiscriminantAnalysis(BaseEstimator, ClassifierMixin):


		# Test the actual attribute:
		clf = QuadraticDiscriminantAnalysis(store_covariances=True).fit(X6, y6)

[MRG+1] Issue#7998 : Consistent parameters between QDA and LDA #8130

[MRG+1] Issue#7998 : Consistent parameters between QDA and LDA #8130

Conversation

mrbeann commented Dec 28, 2016

Reference Issue

mrbeann commented Dec 29, 2016

jnothman commented Dec 29, 2016

mrbeann commented Dec 29, 2016

jnothman commented Dec 29, 2016 via email

mrbeann commented Dec 29, 2016

jnothman commented Dec 29, 2016 via email

mrbeann commented Dec 29, 2016

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

mrbeann commented Jan 3, 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

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

mrbeann commented Jan 7, 2017 • edited Loading

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

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

mrbeann commented Jan 10, 2017

mrbeann commented Mar 3, 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

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 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

lesteve 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

mrbeann commented Jan 7, 2017 •

edited

Loading

amueller Jul 31, 2017 •

edited

Loading