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

table.py: fix issue when specifying both column header text and color #2389

Merged
merged 14 commits into from Feb 1, 2014
Merged

Conversation

bjcohen
Copy link
Contributor

@bjcohen bjcohen commented Sep 6, 2013

Row offset in table construction function was missed in case of specifying both text and color for column headers,
so the first data row was overwritten by the header row.

@@ -506,14 +506,14 @@ def table(ax,
if rowLabels is not None:
assert len(rowLabels) == rows

offset = 0
offset = 1
Copy link
Member

Choose a reason for hiding this comment

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

Could you provide a comment here explaining the logic. Something along the lines of "Assume the table has a column label row, so offset by one". I am not familiar enough with the tables feature to understand the oddity of having blank column labels if column colors are specified, so an explanation there would be useful as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - TBH I don't understand the use case for that either, but that's the way the code worked previously. I was really only interested in fixing the case where both color and text were specified.

@Tillsten
Copy link
Contributor

Looks all good! In the long run tests for the table module would nice to have.

@Tillsten
Copy link
Contributor

@mdboom @pelson Can someone of you have a look at this? It fixes two real bug and i think is ready to merge.

@pelson
Copy link
Member

pelson commented Sep 17, 2013

@bjcohen - this could do with a simple test - even if it doesn't result in a full image test. Would you mind adding something?

@bjcohen
Copy link
Contributor Author

bjcohen commented Sep 18, 2013

Sure - I'll write something up when I get a chance.

bjcohen and others added 5 commits September 28, 2013 10:52
row offset in table construction function was missed in case of specifying both text and color for column headers
1) Comment to clarify what we are offsetting and why.

2) Switched `rows' and `cols' in code to create header rows with
   color but no text, so our lists of empty strings have the
   right length.
@bjcohen
Copy link
Contributor Author

bjcohen commented Sep 28, 2013

@Tillsten @mdboom @pelson I wrote some simple tests for this, do these look ok?

@Tillsten
Copy link
Contributor

looks good, lgtm

@pelson
Copy link
Member

pelson commented Oct 18, 2013

The tests are good, but in truth they are pretty heavy-weight IMHO. Do you have any ideas how we may reduce the overall test overhead for this change (perhaps by mocking some of the internals?).

@Tillsten
Copy link
Contributor

Could you maybe elaborate the point? The test is more or less a simple example, so only way to minimize the test would be just using subplots instead new figures.

@tacaswell
Copy link
Member

@Tillsten I think putting all of the tables in the same figure would be a good idea (only one image test instead of 5)

@mdboom
Copy link
Member

mdboom commented Oct 30, 2013

👍 from me -- once @tacaswell's suggestion of a single test image is implemented.

mdboom and others added 7 commits November 27, 2013 04:35
Refactor mechanism for saving files.
row offset in table construction function was missed in case of specifying both text and color for column headers
1) Comment to clarify what we are offsetting and why.

2) Switched `rows' and `cols' in code to create header rows with
   color but no text, so our lists of empty strings have the
   right length.
@tacaswell
Copy link
Member

@Tillsten Will you have time to merge the test images in the near future? It would be great to get this merged before 1.4 is cut out.

@bjcohen
Copy link
Contributor Author

bjcohen commented Jan 13, 2014

I will rewrite the test to merge the images later today

tacaswell added a commit that referenced this pull request Feb 1, 2014
table.py: fix issue when specifying both column header text and color
@tacaswell tacaswell merged commit be3da1a into matplotlib:master Feb 1, 2014
@tacaswell
Copy link
Member

@bjcohen Sorry I missed that you had added more commits. Thanks for the patch!

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

6 participants