Addressing issue #590:Adding train alpha and test alpha to residual #806

naresh-bachwani · 2019-04-05T18:16:18Z

The following changes were made to Residuals referenced in #590:

Added new parameters train_alpha and test_alpha to specify the opacity of train and test data respectively in residuals.py
Modified the test_residuals.py

The result obtained is attached below:

lwgray · 2019-04-06T16:08:51Z

yellowbrick/regressor/residuals.py

-    alpha : float, default: 0.75
-        Specify a transparency where 1 is completely opaque and 0 is completely
-        transparent. This property makes densely clustered points more visible.
+    train_alpha : float, default: 1


Any particular reason you didn't keep alphas set to default: 0.75

No particular reasons for that. It's more of a personal choice. It can be set as per the yellowbrick standards.

lwgray

I have gone through residuals.py and made a few comments but overall you did a very good job. I will look over the test file and get back to you asap.

lwgray · 2019-04-06T18:31:23Z

yellowbrick/regressor/residuals.py

    visualizer = ResidualsPlot(
        model=model, ax=ax, hist=hist, train_color=train_color,
-        test_color=test_color, line_color=line_color, alpha=alpha,
+        test_color=test_color, line_color=line_color, train_alpha=train_alpha,test_alpha=test_alpha,


The 3rd line is a bit long... you could shorten it as such

visualizer = ResidualsPlot( model=model, ax=ax, hist=hist, train_color=train_color, test_color=test_color, line_color=line_color, alpha=alpha. train_alpha=train_alpha,test_alpha=test_alpha, **kwargs )

lwgray · 2019-04-06T18:49:27Z

yellowbrick/regressor/residuals.py

-                 test_color='g', line_color=LINE_COLOR, alpha=0.75,
-                 **kwargs):
+                 test_color='g', line_color=LINE_COLOR, train_alpha=1,
+                 test_alpha=1,**kwargs):


Again here, we might want to set alphas to 0.75

lwgray · 2019-04-06T18:52:32Z

yellowbrick/regressor/residuals.py

@@ -422,7 +429,10 @@ def __init__(self, model, ax=None, hist=True, train_color='b',
        # Store labels and colors for the legend ordered by call
        self._labels, self._colors = [], []

-        self.alpha = alpha
+        self.alpha = {


This is a minor suggestion and if you don't want to change it, it's ok. Maybe we could change the variable name to self.alphas to note that the variable contains multiple alphas

lwgray · 2019-04-06T18:56:25Z

yellowbrick/regressor/residuals.py

@@ -593,7 +605,8 @@ def residuals_plot(model,
                   test_color='g',
                   line_color=LINE_COLOR,
                   random_state=None,
-                   alpha=0.75,
+                   train_alpha=1,


train and test alpha should be default of 0.75

lwgray · 2019-04-06T19:56:34Z

how did you produce your images above? I noticed that they are missing legends

lwgray · 2019-04-06T20:06:46Z

A side note for future contributions, you merged develop:develop while our branching conventions are to merge from a feature branch. https://www.scikit-yb.org/en/develop/contributing/getting_started.html#branching-convention

naresh-bachwani · 2019-04-07T10:47:30Z

While producing the above results, I did not use poof(). The legends will show up once poof() is called.

naresh-bachwani · 2019-04-07T10:49:00Z

I have made all the recommended changes.

lwgray

You have done an excellent job with this PR. We still need to address why the tolerance was changed. There will be a secondary review by @rebeccabilbro

tests/test_regressor/test_residuals.py

lwgray · 2019-04-06T21:52:03Z

tests/test_regressor/test_residuals.py

        )
-
+        alpha = {


great job of adding this here.

lwgray · 2019-04-07T15:14:43Z

tests/test_regressor/test_residuals.py

@@ -369,4 +372,4 @@ def test_alpha_param(self, mock_sca):
        # Test that alpha was passed to internal matplotlib scatterplot
        _, scatter_kwargs = visualizer.ax.scatter.call_args
        assert "alpha" in scatter_kwargs
-        assert scatter_kwargs["alpha"] == 0.3
+        assert scatter_kwargs["alpha"] == 0.75


Awesome, this shows a good understanding that alpha is 0.3 after visualizer.fit then changes to 0.75 after visualizer.score

rebeccabilbro

Thanks for the contribution @naresh-bachwani — I think these adjustments to the residuals plot will make it much easier to distinguish the training and test points independently, particularly when they are very densely clustered! I've added a note to the question about the test tolerances and had another minor request. Once those edits are in, this should be good to merge!

tests/test_regressor/test_residuals.py

yellowbrick/regressor/residuals.py

lwgray · 2019-04-08T23:11:08Z

@naresh-bachwani I am getting ready to merge your PR, In doing so I've updated your branch with recent changes on yellowbrick develop. You'll need to do a pull in order to incorporate these changes locally before you push anything else to this branch

Adding alpha param to residual

c3c3203

bbengfort added the ready label Apr 5, 2019

bbengfort mentioned this pull request Apr 6, 2019

Effect Plot for Linear Models #604

Open

lwgray reviewed Apr 6, 2019

View reviewed changes

naresh-bachwani added 2 commits April 7, 2019 16:12

Update residuals.py

53c3c9b

Update test_residuals.py

99daa1e

lwgray reviewed Apr 7, 2019

View reviewed changes

lwgray mentioned this pull request Apr 7, 2019

Addressing issue #589: Adding alpha params to PCA #807

Closed

rebeccabilbro requested changes Apr 8, 2019

View reviewed changes

tests/test_regressor/test_residuals.py Outdated Show resolved Hide resolved

yellowbrick/regressor/residuals.py Outdated Show resolved Hide resolved

naresh-bachwani and others added 4 commits April 8, 2019 23:29

Changed tolerance back to 1

9f81216

Minor changes

bea0105

Merge branch 'develop' into develop

804b8f5

Merge branch 'develop' into develop

5bf0b9b

rebeccabilbro approved these changes Apr 8, 2019

View reviewed changes

lwgray merged commit e4f9689 into DistrictDataLabs:develop Apr 8, 2019

lwgray removed the ready label Apr 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing issue #590:Adding train alpha and test alpha to residual #806

Addressing issue #590:Adding train alpha and test alpha to residual #806

naresh-bachwani commented Apr 5, 2019 •

edited

lwgray Apr 6, 2019

naresh-bachwani Apr 6, 2019

lwgray left a comment

lwgray Apr 6, 2019

lwgray Apr 6, 2019

lwgray Apr 6, 2019

lwgray Apr 6, 2019

lwgray commented Apr 6, 2019

lwgray commented Apr 6, 2019

naresh-bachwani commented Apr 7, 2019

naresh-bachwani commented Apr 7, 2019

lwgray left a comment •

edited

lwgray Apr 6, 2019

naresh-bachwani Apr 7, 2019

lwgray Apr 7, 2019

naresh-bachwani Apr 7, 2019

rebeccabilbro left a comment

lwgray commented Apr 8, 2019 •

edited

Addressing issue #590:Adding train alpha and test alpha to residual #806

Addressing issue #590:Adding train alpha and test alpha to residual #806

Conversation

naresh-bachwani commented Apr 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwgray 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

lwgray commented Apr 6, 2019

lwgray commented Apr 6, 2019

naresh-bachwani commented Apr 7, 2019

naresh-bachwani commented Apr 7, 2019

lwgray 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

rebeccabilbro left a comment

Choose a reason for hiding this comment

lwgray commented Apr 8, 2019 • edited

naresh-bachwani commented Apr 5, 2019 •

edited

lwgray left a comment •

edited

lwgray commented Apr 8, 2019 •

edited