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
Conversation
@@ -506,14 +506,14 @@ def table(ax, | |||
if rowLabels is not None: | |||
assert len(rowLabels) == rows | |||
|
|||
offset = 0 | |||
offset = 1 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Looks all good! In the long run tests for the table module would nice to have. |
@bjcohen - this could do with a simple test - even if it doesn't result in a full image test. Would you mind adding something? |
Sure - I'll write something up when I get a chance. |
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.
looks good, lgtm |
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 |
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. |
@Tillsten I think putting all of the tables in the same figure would be a good idea (only one image test instead of 5) |
👍 from me -- once @tacaswell's suggestion of a single test image is implemented. |
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.
@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. |
I will rewrite the test to merge the images later today |
table.py: fix issue when specifying both column header text and color
@bjcohen Sorry I missed that you had added more commits. Thanks for the patch! |
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.