[WIP] Addressing issue 874: Added Projection Visualizer #908

naresh-bachwani · 2019-07-04T11:59:06Z

This PR handles #874 . I updated the tests/features/base.py, which now contains ProjectionVisualizer. It unifies the functionality of PCA and Manifold. It contains the following features:

It handles both discrete and continuous variables.
It handles both 2d and 3d plots.
It manually draws colorbar for 2d plots.

TODOs and questions

Update to handle more features(like LabelEncoder)
Add docstring.
Create tests

bbengfort

@naresh-bachwani good start - and a lot of good things for discussion! Let's keep pressing forward, making sure we've got docstrings and tests to go along with this visualizer as well!

yellowbrick/features/base.py

bbengfort · 2019-07-08T14:23:25Z

yellowbrick/features/base.py

+## Projection Visualizers
+##########################################################################
+
+class ProjectionVisualizer(DataVisualizer):


Perhaps we should put this in yellowbrick/features/projection.py? base.py is getting a little full at this point!

Also to wrap this up, I'd like to make sure we have a complete docstring that describes what this base class does for other yellowbrick developers.

I will write the docstring and move it to projection.py!

As I'm in the airport I'm having trouble with my Internet connection and can't easily check the source code - is there already a projection.py? (I'm hoping there is not).

No, currently we do not have projection.py!

yellowbrick/features/base.py

bbengfort · 2019-07-08T14:38:04Z

yellowbrick/features/base.py

+
+        elif self._target_color_type == TargetType.CONTINUOUS:
+            if(self.proj_dim==3):
+                self.fig.colorbar(self._scatter, ax=self.ax)


We can't rely on there being a self.fig on the visualizer. In this code because that only happens if proj_dim == 3; but generally since we won't have a figure object to work with.

bbengfort · 2019-07-08T14:39:02Z

yellowbrick/features/base.py

+            self._scatter = self.ax.scatter(X[:,0], X[:,1], **scatter_kwargs)
+
+        if self.proj_dim == 3:
+            self.fig = plt.figure()


Rather than creating a new figure, I'd prefer if we used plt.gcf (get current figure) - and also I don't think we should store the fig on the visualizer as discussed in #826.

bbengfort · 2019-07-08T14:39:42Z

yellowbrick/features/base.py

+
+        if self.proj_dim == 3:
+            self.fig = plt.figure()
+            self.ax = self.fig.add_subplot(111, projection='3d')


What about the case where the user has passed in an Axes object? Let's refactor this to override the Visualizer.ax property and do appropriate checks as needed.

Hey @bbengfort,
Can you elaborate on what is needed here? I understand that when user passes Axes we get axes lines on the plot. To deal with that we just need to clear the figure and redefine the Axes. Is that what you mean here?

So a couple of things here, maybe I'm misunderstanding - but there is a difference between Axes and axis spines (see anatomy of a matplotlib plot). The Axes has nothing to do with lines on the plot, it is a part of the figure that is drawn upon.

Consider the following use case:

fig, axes = plt.subplots(ncols=2, projection='3d', figsize=(18,6)) Manifold(ax=axes[0], projection='3d').fit_transform(X, y) PCA(ax=axes[1], projection='3d').fit_transform(X, y)

In this code, the figure is composed of two subplots represented by two Axes objects. The user is asking the Manifold visualization to be drawn on the left axes and the PCA visualization to be drawn on the right axes.

Because of this we can't just create a figure or an axes on our own - we have to respect what the user passes in if they do, and only create the axes if nothing has been supplied.

Does that help clear up the confusion?

Ok! Great! But what if the user passes non-3d axes? We can't draw 3d plots on that! Also, I could not find methods to convert 2d to 3d axes. Will we raise an error in this case?

This is a very good point - we should see what happens! If matplotlib raises a sensible error, then we'll keep that error, or wrap it in a YellowbrickError. Otherwise matplotlib might just plot it in 2D, I'm not sure - so let's find out!

yellowbrick/features/base.py

bbengfort · 2019-07-08T14:40:25Z

yellowbrick/features/base.py

+        X = self.transformed_features_
+        scatter_kwargs = self._determine_scatter_kwargs(y);
+
+        if self.proj_dim == 2:


Should we divide this up into _draw_2d and _draw_3d methods to make the organization a bit easier?

We can do this, but I think that the method is quite small and understandable.

Ok, in that case let's leave it as it is!

bbengfort · 2019-07-09T16:37:26Z

yellowbrick/contrib/missing/base.py

@@ -60,8 +60,8 @@ def fit(self, X, y=None, **kwargs):

        self.y = y

-        super(MissingDataVisualizer, self).fit(X, y, **kwargs)


We probably do still want the call to super right? To ensure that the target type is identified and that the class and feature names are collected?

Bump. Or is this going into #923?

Yup! I think that dealing with this in #923 will make more sense.

tests/test_features/test_base.py

yellowbrick/features/base.py

yellowbrick/features/pcoords.py

yellowbrick/features/projection.py

bbengfort

This is really coming together, thank you so much for your hard work on this! The tests look great and are really well done.

I think the only remaining thing is the transfomer property on the visualizer. Right now it's accessed on fit() and transform() but without any other indication that it was supposed to be there. I had considered offering suggestions of creating a set_transformer function or using self._transfomer ... however I think that these are going to cause problems with our subclasses as well.

Instead what I recommend is that we remove the fit() and transform() methods completely from this visualizer. They don't really do anything except call the self.transformer method and our subclasses will likely modify that behavior. However, the subclasses will need to call fit super in order to set the classes, features, and detect the target type; so having this in the middle is going to cause problems when there is no self.transformer on the class. All the ProjectionVisualizer needs to know is how to draw Xp with 2 or 3 dimensions, and then the subclasses can call draw in their fit or transform methods.

This will necessarily change your testing methodology. So instead of relying on the transformer property in tests, you'll set up the subclass just as a Yellowbrick developer would use it:

class MockVisualizer(ProjectionVisualizer):

    def __init__(self, n_components=2):
        self.transformer = PCA(n_components, random_state=2019)
  
    def fit(self, X, y):
        super(MockVisualizer, self).fit(X, y)
        self.transformer.fit(X)
        return self 
    
     def transform(self, X, y):
         Xp = self.transformer.transform(X)
         self.draw(Xp, y)
         return Xp


class TestProjectionVisualizer(VisualTestCase):
    
    def test_discrete_plot(self):
        X, y = self.discrete
        oz = MockVisualizer()
        Xp = oz.fit_transform(X, y)
        self.assert_images_similar(oz)

Note that this does make some of my comments in the test section obsolete.

bbengfort · 2019-07-16T12:17:21Z

yellowbrick/contrib/missing/base.py

@@ -60,8 +60,8 @@ def fit(self, X, y=None, **kwargs):

        self.y = y

-        super(MissingDataVisualizer, self).fit(X, y, **kwargs)


Bump. Or is this going into #923?

yellowbrick/features/pcoords.py

tests/test_features/test_projection.py

yellowbrick/features/projection.py

bbengfort · 2019-07-16T13:31:03Z

@naresh-bachwani when you've made the changes and are ready - let me know and I'll do the final run through of the code with the test images, etc.

naresh-bachwani · 2019-07-16T17:45:37Z

Well I like your suggestion related to fit() and transform(). I will make the necessary changes and let you know!

bbengfort

This looks great and I'm glad the new MockVisualizer was able to smooth things to get the tests to pass! I have some minor cleanup that I'm going to push and then when the tests pass I'll get this merged in. I'll also quickly toss-up #923 for you to review so that you can have that to work with as you move forward.

In terms of organizing things for the next step, I think what I'd like to do is to move projection.py to yellowbrick/features/projection/base.py and move manifold.py and pca.py to yellowbrick/features/projection but make sure they are directly importable from yellowbrick.features (e.g. make sure the import is correct in __init__.py. We can then move decomposition.py to yellowbrick/features/pca.py and have ExplainedVariance be a tad more accessible. Don't worry - we'll go over this as you move to the next PR!

Great work!

naresh-bachwani and others added 3 commits July 4, 2019 17:17

Added Projection Visualizer

9b53e13

Merge branch 'develop' into projectviz

f9b4f8d

Added transformer param and changed the signature of draw

e2087eb

rebeccabilbro assigned bbengfort Jul 8, 2019

bbengfort reviewed Jul 8, 2019

View reviewed changes

naresh-bachwani and others added 7 commits July 9, 2019 03:58

Updated data visualizer and move Projection to new file while updating

fb1d3e7

Merge branch 'develop' into projectviz

d171ab3

Updated RadViz

5f91e88

Minor changes

af7b015

Few minor changes

79baedc

Added docstring and changed a a few iports

f0b5614

few minor changes

f11e8cb

bbengfort reviewed Jul 9, 2019

View reviewed changes

tests/test_features/test_base.py Show resolved Hide resolved

bbengfort reviewed Jul 9, 2019

View reviewed changes

yellowbrick/features/base.py Show resolved Hide resolved

bbengfort reviewed Jul 9, 2019

View reviewed changes

yellowbrick/features/pcoords.py Show resolved Hide resolved

bbengfort reviewed Jul 9, 2019

View reviewed changes

yellowbrick/features/projection.py Outdated Show resolved Hide resolved

naresh-bachwani added 6 commits July 10, 2019 03:22

Overrides the ax from base and modifies

14d4db4

Small changes

98ffe32

few more

e104b96

corrected typo

be2e71e

Added test and images

9cbff21

Minor changes in tests

4ae3c03

bbengfort requested changes Jul 16, 2019

View reviewed changes

naresh-bachwani and others added 2 commits July 17, 2019 16:47

Made suggested changes

54a6822

Merge branch 'develop' into projectviz

8035ba4

bbengfort approved these changes Jul 17, 2019

View reviewed changes

docstring cleanup

a16f4fc

bbengfort merged commit 21eb9d2 into DistrictDataLabs:develop Jul 17, 2019

naresh-bachwani deleted the projectviz branch July 22, 2019 17:39

naresh-bachwani mentioned this pull request Nov 9, 2019

PCA visualizer is not rendering 3D plot #984

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Addressing issue 874: Added Projection Visualizer #908

[WIP] Addressing issue 874: Added Projection Visualizer #908

naresh-bachwani commented Jul 4, 2019 •

edited

bbengfort left a comment

bbengfort Jul 8, 2019

bbengfort Jul 8, 2019

naresh-bachwani Jul 8, 2019

bbengfort Jul 8, 2019

naresh-bachwani Jul 8, 2019

bbengfort Jul 8, 2019

bbengfort Jul 8, 2019

bbengfort Jul 8, 2019

bbengfort Jul 8, 2019

naresh-bachwani Jul 9, 2019

bbengfort Jul 9, 2019

naresh-bachwani Jul 9, 2019

bbengfort Jul 9, 2019

bbengfort Jul 8, 2019

naresh-bachwani Jul 8, 2019

bbengfort Jul 9, 2019

bbengfort Jul 9, 2019

bbengfort Jul 16, 2019

naresh-bachwani Jul 17, 2019

bbengfort left a comment

bbengfort Jul 16, 2019

bbengfort commented Jul 16, 2019

naresh-bachwani commented Jul 16, 2019

bbengfort left a comment

		@@ -60,8 +60,8 @@ def fit(self, X, y=None, **kwargs):

		self.y = y

		super(MissingDataVisualizer, self).fit(X, y, **kwargs)

[WIP] Addressing issue 874: Added Projection Visualizer #908

[WIP] Addressing issue 874: Added Projection Visualizer #908

Conversation

naresh-bachwani commented Jul 4, 2019 • edited

TODOs and questions

bbengfort 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbengfort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbengfort commented Jul 16, 2019

naresh-bachwani commented Jul 16, 2019

bbengfort left a comment

Choose a reason for hiding this comment

naresh-bachwani commented Jul 4, 2019 •

edited