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

Implemented issue 4044. Created a Cell subclass, SciCell, to override th... #4245

Closed
wants to merge 6 commits into from

Conversation

anthonycho
Copy link

...e default draw function. SciCell draws a polygon (line) instead of the rectangle. Extended Table to use the new SciCell class. #4044

… the default draw function. SciCell draws a polygon (line) instead of the rectangle

def add_cell(self, row, col, *args, **kwargs):
""" Add a cell to the table. """
xy = (0, 0)

cell = Cell(xy, *args, **kwargs)
if self._cellType == 'default':
Copy link
Member

Choose a reason for hiding this comment

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

I would use a class-level dictionary to do this look up

@tacaswell tacaswell added this to the next point release milestone Mar 19, 2015
@tacaswell
Copy link
Member

This is a great start! I left a bunch of comments, some are style related some are code-design related.

I know it may seem a bit intimidating/hostile, but don't be discouraged. I am happy to work with you through the process of getting this PR ready to merge and answer any questions you have.


"""

def __init__(self, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

You don't actually need the __init__ here if all you do is call the base class __init__

@anthonycho
Copy link
Author

Hi Thomas, just a quick question, did I implement the class level dictionary correctly? That was the only confusing part for me because I didn't know what the value should be (it would be a list if we didn't need a pairing). Thanks!

@anthonycho
Copy link
Author

Is it possible to rerun travis without committing? 3efbf16 failed the unit tests but fbdc81a passed. The only difference between the two are whitespace changes.

@jenshnielsen
Copy link
Member

@anthonycho I restarted the failing job. Looks like it was just a random glitch in the python 2.6 tests

@@ -211,12 +242,15 @@ def __init__(self, ax, loc=None, bbox=None, **kwargs):
self.set_clip_on(False)

self._cachedRenderer = None
self._cellType = 'default'
self._cellCreation = {"default": Cell, "scicell": SciCell}
Copy link
Member

Choose a reason for hiding this comment

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

This is an instance level attribute. A class level would look something like

class foo(object):
    bar = {'a': 1, 'b': 2}
    def __init__(self, lab):
        if lab not in self.bar:
            raise ValueError("invalid key")
       self.label = self.bar[lab]

The bar attribute is shared between all instances of the class so changing it results in it changing for all instances. This can be useful if you want to set up a registry of available Cell types.

…es a class level dictionary and modified code to use it.
@anthonycho
Copy link
Author

@jenshnielsen Thanks!

@tacaswell OK, thanks for the feedback. I've made the necessary changes to use the new class dictionary.

@@ -181,6 +212,7 @@ class Table(Artist):

FONTSIZE = 10
AXESPAD = 0.02 # the border between the axes and table edge
AVAILABLECELLTYPES = {"default": 0, "scicell": 1}
Copy link
Member

Choose a reason for hiding this comment

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

You can put the Cell classes as values in this dictionary.

@anthonycho
Copy link
Author

@tacaswell to clear up some confusion, we are going with pull request #4259 ?

@tacaswell
Copy link
Member

Can you work that out with @dkua and @KaiSong2014 ?

@anthonycho
Copy link
Author

@tacaswell we are going with @dkua pr. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants