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.

Sign up for GitHub

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

Jump to bottom

Fix bug in plot.py and add missing test #897

Merged
merged 18 commits into from
Mar 12, 2017
Merged

Fix bug in plot.py and add missing test #897

merged 18 commits into from
Mar 12, 2017

Conversation

meatballs
Copy link
Member

Fixes #894

meatballs and others added 16 commits March 11, 2017 14:04
@meatballs
[#894] Add tests for default_cmap
a161b95
@meatballs
[#894] Fix PEP8 errors and remove __main__ condition
766df5d
@meatballs
[#894] Fix PEP8 errors
8d079a5
@meatballs
[#894] Correct exptected result for matplotlib 1.7
d9a46f1
@meatballs
[#894] Refactor default_cmap function
e600d75
@meatballs
[#894] Fix PEP8 and mypy errors
331390f
@meatballs
[#894] Add matplotlib version into call to default_cmap
7dffc90
@meatballs
[#894] Fix mypy syntax error
de2e59e
@drvinceknight
Fix sphinx build bug.
7d46b8f
@drvinceknight
Use distutils version comparison.
35e9801 
I also reverted back to use if statements, which felt simpler in this (2
option) case.
@drvinceknight
Add another test.
57cdb80
@drvinceknight
Fix bug: correct plotting on axes.
90be7ca 
Closes #896
@drvinceknight
Remove anti pattern.
4d19db8
@meatballs
[#894] Merge commit '4d19db828ea0ee9d3e08b6e767aa2fe0bb163b18' into i…
c85444e 
…ssue-894

* commit '4d19db828ea0ee9d3e08b6e767aa2fe0bb163b18':
  Remove anti pattern.
  Add another test.
  Use distutils version comparison.
@meatballs
[#894] Revert "Remove anti pattern."
2427db6 
This reverts commit 4d19db8.
@drvinceknight
Add missing test for heatmap and save_all_plots.
f3c3afd 
Also change some ResultSet reads to not display a progress bar.

This pushes the coverage of this file to 100%.
drvinceknight
drvinceknight approved these changes Mar 12, 2017
View reviewed changes
Copy link
Member

@drvinceknight drvinceknight left a comment

Choose a reason for hiding this comment

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

This all looks good to me @meatballs, I've just pushed f3c3afd which caught two more things (and pushes the plot file coverage to 100%):

  • There was the same missing test/bug for the axes for the heatmap (I missed it yesterday)
  • There was a missing test for a progress bar run.

I also made some result sets build without a progress bar (just to clean up the output).

@drvinceknight
Copy link
Member

Just pushed another commit which ensures the colour bar is correctly placed too.

@marcharper
Copy link
Member

Looks fine to me. I don't want to squash all these commits... You might consider an interactive rebase git rebase -i onto upstream, removing the revert commit and the original commit by omitting them from the list. Then force push to the branch. If you are both fine with the unnecessary commits then just merge.

@drvinceknight
Copy link
Member

If you are both fine with the unnecessary commits then just merge.

I'm agnostic, whatever Owen prefers to do.

@meatballs
Copy link
Member Author

I'd go with just merging with the reversal et al

@drvinceknight drvinceknight merged commit bcb0013 into master Mar 12, 2017
@drvinceknight drvinceknight deleted the issue-894 branch March 12, 2017 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drvinceknight drvinceknight drvinceknight approved these changes

Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@meatballs @drvinceknight @marcharper