Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor all Visualizers to move fig/ax hooks to base class #62

Closed
rebeccabilbro opened this issue Oct 6, 2016 · 4 comments
Closed
Assignees
Labels
level: intermediate python coding expertise required priority: high should be done before next release
Milestone

Comments

@rebeccabilbro
Copy link
Member

rebeccabilbro commented Oct 6, 2016

Move fig/ax hooks to __init__ in base Visualizer class.

@rebeccabilbro rebeccabilbro self-assigned this Oct 6, 2016
@rebeccabilbro rebeccabilbro added level: intermediate python coding expertise required priority: medium can wait until after next release labels Oct 6, 2016
@bbengfort bbengfort added priority: high should be done before next release and removed priority: medium can wait until after next release labels Oct 7, 2016
@bbengfort
Copy link
Member

bbengfort commented Oct 7, 2016

So the primary idea here is that every single visualizer will need access to some matplotlib axes object to draw on (axes is where most of the plotting functions are). It also seems like they should have access to the figure object, though it is very unusual to modify that in any way -- so for now, let's focus on the ax object.

When you look at Seaborn and Pandas plotting functions (not classes) most of them take as input an axes and return an axes. This allows you to chain multiple functions together:

sns.heatmap().gridplot() 

Which is useful if you want to draw multiple lines or text or customize the graph. Although this api is not exactly what we'll use - what I'm trying to demonstrate is that the axis is a reusable thing until the figure is shown (or poofed).

Every visualizer needs a hook to some axis to draw on, and if it can be passed in, then the same axis can be used in multiple visualizers (for example to draw a model curve on a prediction error plot or something like that). However, if it's not passed in, then it will create a new axis by using the plt.gca() (get current access) function.

Since we're doing a class based API and we know that every single visualizer needs an access, we can do this work once at the root of the API hierarchy - at Visualizer:

class Visualizer(BaseEstimator):

    def __init__(self, ax=None, **kwargs):
      self.ax = ax 

Subclasses need to do something to the effect of:

class SubVisualizer(Visualizer):

    def __init__(self, ax=None, **kwargs):
        super(SubVisualizer, self).__init__(ax=ax, **kwargs)

Though you could just leave ax in the generic kwargs and pass them up, I think this is so vital that every Visualizer subclass should have a constructor signature that looks like this. However, how this happens is open to discussion, the above is only one way.

The next step is that you'll see the following code in my draw methods on subclasses:

class SubVisualizer(Visualizer):

    def draw(self, **kwargs):
        if self.ax is None:
            self.ax = plt.gca() 

This is so routine, it should also be a method on the Visualizer (able to be overridden if the gca call or axis construction needs to happen differently):

class Visualizer(BaseEstimator):

    def gca(self):
        if self.ax is None:
            self.ax = plt.gca() 
        return self.ax 

    def draw(self, **kwargs):
        ax = self.gca() 

We could instead use the property decorator method:

class Visualizer(BaseEstimator):

    def __init__(self, ax=None, **kwargs):
      self.ax = ax 

    @property
    def ax(self):
        if self._ax is None:
            self._ax = plt.gca() 
        return self._ax

    @ax.setter 
    def ax(self, value):
        self._ax = value 

    def draw(self, **kwargs):
        self.ax.plot(x, y, c='r')   # If ax is None it will automatically be created. 

If something special needs to be done in gca as in radviz.py#L161, then the subclass can override ax() or gca() to do it's special work.

These two different APIs set a tone for how users might extend and create new things with Yellowbrick - both are good, but different choices. There are other options as well, but these are the two most prominent.

@bbengfort bbengfort added this to the Backlog milestone Oct 8, 2016
@bbengfort
Copy link
Member

Let's save this refactor for the next version and not worry about it for this weekend! I'll try to clarify my comments from above.

@rebeccabilbro rebeccabilbro changed the title Refactor ClassificationScoreVisualizers to move fig/ax hooks Refactor all Visualizers to move fig/ax hooks to base class Oct 8, 2016
@rebeccabilbro
Copy link
Member Author

@bbengfort bbengfort modified the milestones: Version 0.3.2, Backlog Oct 13, 2016
@rebeccabilbro rebeccabilbro added in progress label for Waffle board and removed ready labels Oct 30, 2016
@rebeccabilbro
Copy link
Member Author

See commit 5f7ab81

bbengfort added a commit that referenced this issue Nov 5, 2016
@rebeccabilbro rebeccabilbro removed the in progress label for Waffle board label Nov 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
level: intermediate python coding expertise required priority: high should be done before next release
Projects
None yet
Development

No branches or pull requests

2 participants