Implementation of Issue #4044. Added ScientificTable and ScientificCell subclasses.

[dkua]

This implements the requested feature in issue #4044

Two subclasses, ScientificTable and ScientificCell, were created of Table and Cell respectively. An extra kwarg was added to the table() factory function for choosing the type of Table to use.

The factory function and the table types were tested with the test_table_types test in test_table.py. A new test image was also added. The test passes in Ubuntu 12.04 64-bit.

The feature was implemented by having ScientificCell extend Cell with an additional method get_path() which simply returns a new Path instance that doesn't draw a line when moving between the vertices that make up a vertical line.

This works because Cell.draw() calls Rectangle.draw() which calls self.get_path(). However Cell passes into Rectangle.draw() the arguments self, renderer. So the self.get_path() call is actually calling Cell but since it doesn't have that method it uses it's super class' definition which is Rectangle. By creating the new subclass with our own get_path() method we can implement this feature while following the same logic.

[tacaswell]

We now have two implementations of this #4245

[tacaswell]

Looking at this PR and #4245, I like the implementation of the new Cell type is this PR and the cell type selection mechanic from other PR.

[dkua]

Thanks for responding so quickly @tacaswell. The newest commit removes the ScientificTable subclass and implements a similar cell type selection mechanic as you liked.

[dkua]

That's strange. Travis failed this single test: https://travis-ci.org/matplotlib/matplotlib/jobs/55347259#L6121

But running the test_lines and test_table tests myself on my system, they pass.

[tacaswell]

That failure is just travis being flaky. That particular test is verifying that an optimization actually makes things better. If the VM chokes (due to a greedy neighbour or other VM related things) during the timing of the optimized section the test will fail.

I kicked it to restart the 2.7 tests.

[dkua]

Tests pass! Thanks @tacaswell

[ghost]

@tacaswell We've spoken to the groups who contributed the other PRs. We've agreed that this PR provides a sound implementation that we are all happy with. The other PRs have been closed while this remains open - any updates on potentially merging this feature? Thanks!

[tacaswell]

I should have time this weekend unless someone beats me to it.

[ghost]

@tacaswell - thanks for the heads up. Cheers!

[efiring]

Just a quick thought: I suggest you reconsider the names. This is really about two alternative styles, neither of which has anything to do with science. Fundamentally, a table can have horizontal and/or vertical dividers, or no dividers, or dividers in some locations and not in others. If we are never going to have options other than whether to use vertical dividers, then the styles might be "open cells" (what you are calling "Scientific" versus "closed cells". But it might be good to look farther ahead, and lay the groundwork for a more complete set of formatting options.

[dkua]

@efiring Maybe the term VerticalCell and HorizontalCell would be more apt terms respectively for cells with only vertical and only horizontal lines?

[efiring]

Yes, with a cell-based approach, that would be better. But might it make sense to simply have one Cell class that can put lines on any or all of the sides? The general specification would be a string, e.g. 'LRBT' for all sides, 'BT' for bottom and top, etc. Aliases could be 'open' for no lines (''), 'closed' for all sides, 'horizontal' for horizontal lines ('BT'), and 'vertical' for vertical lines ('LR').

[dkua]

@efiring Wouldn't the Cell-based approach potentially be simpler in the case that the logic for each type of Cell is more complex than simply swapping the Path instance? Instead of having all of that logic inside of a single Cell class we have multiple subclasses of Cell. Do end-users interact with the Cell types directly?

[efiring]

No, I expect users will interact with the table, not the Cell types. But I also expect that the next request from users will be for the ability to control the cell type on a cell-by-cell basis, not just for the table as a whole. Based on a very quick look, I think one could allow (but not require) an array of cell types, just as there is an array of text entries. Either way, the Table is drawing cells with properties. If the Cell (or a single FancyCell subclass) accepts a boxstyle property (or perhaps even a dictionary of properties), then there is no need for more and more subclasses.
I hope I'm not leading you astray; I have never used the table or studied the code. But I'm concerned about locking in a new API (public classes, etc.). As mpl matures, it gets harder and harder to modify the API, so more up-front design thought is warranted. I recognize that this makes contributions like this more difficult.

[dkua]

@efiring At the moment there isn't a way for users to declare exactly what kind of Cell they want at (x, y) position. It seems that for a single Table, one type of Cell is used throughout. It might be a premature optimization to give the Table class and table() factory function the ability to declare what Cell is used for what position. Maybe it's best left for some future point in time to change the API for selecting separate kinds of Cells.

[efiring]

Agreed; I'm not arguing that you need to do all of that, but rather that what you do now should leave the door open as much as possible. That's why I am thinking in terms of having a minimum number of Cell subclasses (like one) instead of adding on the specialized HorizontalCell and VerticalCell (or just one of these). Once they exist, they have to be supported for quite a while.

[zairm]

@efiring We've created another version of the code to work as you've suggested. Here is the new code.

To provide a quick summary:

• An arguement to the table() function to specifies which cell lines should show.
  (for example, "LRBT" (in any order) creates a table where each cell has left, right, bottom, and top boundary visible. All aliases that were suggested are also implemented.)
• The Table instance stores the above information.
• With creation of a new cell instance from add_cell() in Table (the cell is now a FancyCell - A subclass of Cell), the cell is passed in the mentioned "LRBT" type information so that the path returned by get_path() allows for the appropriate lines to be in/visible. In the future, if individual cell edges need to be made in/visible it may be done so in FancyCell. Of course, this is because the cell instance stores its own "LRBT" type information.

Please let us know if this is what you had in mind.

[efiring]

Yes, that is what I had in mind, thank you. It looks like you might be able to factor out the validation of the edge letter string; typically, if there is a setter, then this can be used in the init to handle validation. Also, for the "closed" or "LRBT" case, maybe you need to just use the rectangle; one way or another, the polygon should be closed so that when it is drawn there is no line-end artifact at the starting/ending corner.

[tacaswell]

I also like the new version much better (but have not read it in detail).

[dkua]

@tacaswell @efiring Cleaned up some of the code created by @zairmubashar and added it to this PR. Please have a look and let us know what you think.

[efiring]

Lower case for variable name.

[dkua]

The EDGES variable is also called in the init() and get_path() methods.

[efiring]

Right, I was confused, and had to amend my comment.

[dkua]

@efiring I've added changes to the test that you wanted and changed the default edges type to "closed".

[efiring]

Thank you. @tacaswell or @WeatherGod, any more suggestions?
I think this is ready for entries in doc/users/whats_new/ and doc/api/api_changes/. In both cases there are a README and the existing entries to use as examples. The reason for using individual files for these entries is that it greatly reduces merge conflicts.

[zairm]

@tacaswell @WeatherGod, are there any more suggestions or should we pull together some entries for the docs?

[OceanWolf]

I haven't had a good look though this yet, but from what I see, it looks like we should use this to implement the Legend class. At the moment Legend does all the calculations by hand, something I think the Table class should take care of.

Does Legend currently require any extra features not currently addressed by Table?

[dkua]

@OceanWolf we're not sure. We can look into it. But might that not be a topic for another issue/PR?

[OceanWolf]

@dkua sure thing, I only ask now to double check on the Table API.

[tacaswell]

@OceanWolf Overhauling Legend is definitely out of scope for this PR!

[tacaswell]

trim the extra white space please.

We much not have pep8 compliance on the rest of this file or the test would be failing.

[dkua]

@tacaswell do you mean the extra whitespace inside the _edge_aliases dictionary? I can't see any other whitespace and yeah pep8 isn't failing.

[tacaswell]

I would expect pep8 to require

_edge_aliases = {'open': '',

[dkua]

@tacaswell afaik spaces are allowed after colons inside of dictionaries.

Nothing against it is mentioned in the PEP 8 style guide: https://www.python.org/dev/peps/pep-0008/

And running pep8 on a minimal example and people on SO seem to confirm: http://stackoverflow.com/questions/13497793/python-alignment-of-assignments-style

I'm also at PyCon right now, I could ask people here too

[tacaswell]

Well, learn something every day.

[tacaswell]

👍 Looks good! I like the path-based solution to drawing the edges.

@efiring I think this just needs a whats_new, not an api_changes as I don't think it breaks any existing API.

[efiring]

@tacaswell, good point; I was confused as to when an api_changes entry is needed.

[dkua]

@tacaswell @efiring could you point us to where (or how) we would write up a whats_new entry?

[tacaswell]

https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new

The readme in that folder as an explanation.

On Sun, Apr 12, 2015 at 11:38 AM David Kua notifications@github.com wrote:

@tacaswell https://github.com/tacaswell @efiring
https://github.com/efiring could you point us to where (or how) we
would write up a whats_new entry?

—
Reply to this email directly or view it on GitHub
#4259 (comment)
.

[tacaswell]

I think this is waiting on a what's new entry and that PEP8 issues?

[dkua]

@tacaswell yes it seems just those two issues

[tacaswell]

I am apparently confused about PEP8, so just one issue left 😄

[WeatherGod]

@tacaswell, I could have sworn the same thing until someone called me out
on it on a code review last month. I went to look it up and I couldn't find
the entry that I could have sworn was there explaining the formatting of a
dictionary. Maybe it changed at some point?

On Mon, Apr 13, 2015 at 9:45 PM, Thomas A Caswell notifications@github.com
wrote:

I am apparently confused about PEP8, so just one issue left [image:
😄]

—
Reply to this email directly or view it on GitHub
#4259 (comment)
.

[dkua]

@tacaswell I've added a What's New entry to the docs. We've also added an extra tests for the Cells themselves just for extra assurance that things work.

[tacaswell]

@dkua @zairmubashar Thanks!

[dkua]

@tacaswell @efiring thank you very much.