[MRG + 1] Create a new figure and test each plot type #127 #179

suyashb95 · 2018-10-27T03:00:59Z

move plot() to plotting.py as plot_pdf()
add filename param to plot_pdf()
modify plotting functions to return matplotlib figures
add test_plotting.py and baseline images
import plot_pdf() in __init__
update cli.py to use plot_pdf()
update advanced usage docs to reflect changes

- move `plot()` to `plotting.py` as `plot_pdf()` - modify plotting functions to return matplotlib figures - add `test_plotting.py` and baseline images - import `plot_pdf()` in `__init__` - update `cli.py` to use `plot_pdf()` - update advanced usage docs to reflect changes

suyashb95 · 2018-10-27T03:28:25Z

@vinayak-mehta Any idea why changing the matplotlib backend doesn't help with the Python 2.7 build while the others pass?

suyashb95 · 2018-10-28T05:33:32Z

Build logs

This call to matplotlib.use() has no effect because the backend has already
been chosen; matplotlib.use() must be called *before* pylab, matplotlib.pyplot,
or matplotlib.backends is imported for the first time.
The backend was *originally* set to 'TkAgg' by the following code:
  File "/home/travis/virtualenv/python2.7.14/bin/pytest", line 11, in <module>
    sys.exit(main())

Would moving matplotlib.use('Agg') to __init__ in the tests module help?

vinayak-mehta · 2018-10-28T08:57:22Z

Would moving matplotlib.use('Agg') to init in the tests module help?

Sure, you could try that. But why do you need to specify the backend in the first place? Isn't Agg the default backend? Is this a pytest-mpl requirement?

suyashb95 · 2018-10-28T11:31:05Z

Would moving matplotlib.use('Agg') to init in the tests module help?

Sure, you could try that. But why do you need to specify the backend in the first place? Isn't Agg the default backend? Is this a pytest-mpl requirement?

I don't think Agg is the default backend for the python2.7 build. It fails on not finding a display. More on that here

- use matplotlib rectangle instead of `cv2.rectangle` in `plot_contour()` - set matplotlib backend in `tests/__init__` - update contour plot baseline image - update `test_plotting` with more checks

codecov-io · 2018-10-28T11:53:45Z

Codecov Report

Merging #179 into master will increase coverage by 3.85%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   84.12%   87.98%   +3.85%     
==========================================
  Files          13       13              
  Lines        1235     1248      +13     
  Branches      297      298       +1     
==========================================
+ Hits         1039     1098      +59     
+ Misses        151      101      -50     
- Partials       45       49       +4

Impacted Files	Coverage Δ
camelot/core.py	`91.6% <ø> (+4.09%)`	⬆️
camelot/handlers.py	`82.5% <ø> (ø)`	⬆️
camelot/cli.py	`92% <33.33%> (-1.75%)`	⬇️
camelot/plotting.py	`91.78% <87.17%> (+75.78%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79db6e3...655c4f9. Read the comment docs.

vinayak-mehta · 2018-10-30T13:55:12Z

tests/test_plotting.py

+    for t in text:
+        xs.extend([t[0], t[2]])
+        ys.extend([t[1], t[3]])
+    assert ax.get_xlim() == (min(xs) - 10, max(xs) + 10)


@suyash458 Why do we need additional asserts here? Doesn't pytest compare the whole image which is a sure shot way of knowing whether the images are the same or not?

In case image comparison tests are skipped if the --mpl option is not specified, these asserts would still work. Ultimately though, image comparison is a sure shot test.

Should the --mpl option be added to addopts in setup.cfg ?

Yes and in the Makefile too. After that we shouldn't need to worry about additional asserts.

vinayak-mehta · 2018-10-30T13:56:48Z

camelot/plotting.py

                         [cell.lb[1], cell.rb[1]])
-    plt.show()
+    return fig


@suyash458 Are you returning fig from the plot_* functions only for those additional asserts in the tests?

pytest-mpl's image comparison decorator requires the plot functions to return a matplotlib figure

- remove unnecessary asserts - update setup.cfg and makefile with `--mpl`

vinayak-mehta · 2018-10-31T13:09:38Z

@suyash458 How did you generate the baseline images? Looks like there's an error of 10 units, so the tests are failing with the default tolerance of 2.

suyashb95 · 2018-10-31T13:12:59Z

@vinayak-mehta I'm not sure why the tests fail on Travis even though they pass locally. The baseline images were generated using the same matplotlib backend

I used the --mpl-generate-path=baseline option locally to generate them

vinayak-mehta · 2018-10-31T13:14:54Z

I mean did you generate the plots using the CLI and then saved them from the matplotlib interface? or using the API. I'll try to run the tests locally too.

suyashb95 · 2018-10-31T13:16:41Z

I mean did you generate the plots using the CLI and then saved them from the matplotlib interface? or using the API. I'll try to run the tests locally too.

pytest --mpl-generate-path=baseline
This skipped the image comparison tests and generated the images in a folder called baseline

vinayak-mehta · 2018-11-01T09:05:15Z

So I ran the tests locally and they failed. When I checked the diff images in /tmp I found that the large RMS error is coming from the axes not getting subtracted in the diff somehow. Check out this diff:

I think this difference could be the result of baseline images being generated on a system with some different font/backend/(some other setting) (you said you used 'Agg' though and that is being used on Travis too). I plan to look into what the exact difference is later by inspecting the dpi, dimensions etc. We could resolve this by doing one of the following:

Adding a tolerance parameter, which I've pushed for the time being.
Removing axes from plots. Does matplotlib give such an option?
Generate the baseline images on Travis itself and copy them to the tests directory.

vinayak-mehta · 2018-11-01T09:06:28Z

3 looks the best to me.

suyashb95 · 2018-11-01T12:07:15Z

Option 3 looks like the best bet. I think the large RMS error could be because of different system fonts being used. pytest-mpl has an option to exclude text from the comparison test so I'll try that

update plot tests with `remove_text`

suyashb95 · 2018-11-01T17:52:55Z

Looks like the tests passed after excluding text from the baseline images. The py3.7 build failed because of an AWS timeout

vinayak-mehta · 2018-11-01T18:06:21Z

Restarted.

suyashb95 · 2018-11-02T01:51:28Z

Should we go ahead with this so the tests don't fail on Travis or locally owing to differences in freetype fonts?

vinayak-mehta · 2018-11-02T10:07:14Z

remove_text looks good to me. I'll merge this today.

vinayak-mehta · 2018-11-02T15:27:16Z

@suyash458 Thanks for the contribution!

suyashb95 · 2018-11-02T16:46:24Z

@vinayak-mehta Glad I could help!

suyashb95 added 2 commits October 27, 2018 08:17

Change matplotlib backend for image comparison tests

336fee8

suyashb95 changed the title ~~[MRG] Create a new figure and test each plot type #127~~ [WIP] Create a new figure and test each plot type #127 Oct 28, 2018

Update plotting and tests

5924d95

- use matplotlib rectangle instead of `cv2.rectangle` in `plot_contour()` - set matplotlib backend in `tests/__init__` - update contour plot baseline image - update `test_plotting` with more checks

suyashb95 mentioned this pull request Oct 28, 2018

Create a new figure and test each plot type #127

Closed

suyashb95 changed the title ~~[WIP] Create a new figure and test each plot type #127~~ [MRG] Create a new figure and test each plot type #127 Oct 29, 2018

vinayak-mehta reviewed Oct 30, 2018

View reviewed changes

suyashb95 added 2 commits October 31, 2018 18:15

Update plot tests and config

9dad125

- remove unnecessary asserts - update setup.cfg and makefile with `--mpl`

Add to

0af6517

Add tolerance

837a657

remove text from baseline plots

d0edd43

update plot tests with `remove_text`

Change method name, update docs and add pep8

c83ebc2

vinayak-mehta force-pushed the fix-#127 branch from efe48da to c83ebc2 Compare November 2, 2018 11:42

Update docs

655c4f9

vinayak-mehta changed the title ~~[MRG] Create a new figure and test each plot type #127~~ [MRG + 1] Create a new figure and test each plot type #127 Nov 2, 2018

vinayak-mehta merged commit c0e9235 into atlanhq:master Nov 2, 2018

suyashb95 deleted the fix-#127 branch November 2, 2018 15:50

suyashb95 restored the fix-#127 branch November 2, 2018 16:45

suyashb95 deleted the fix-#127 branch November 2, 2018 16:46

suyashb95 restored the fix-#127 branch November 2, 2018 16:47

vinayak-mehta mentioned this pull request Nov 6, 2018

Can't plot when no tables are found #150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG + 1] Create a new figure and test each plot type #127 #179

[MRG + 1] Create a new figure and test each plot type #127 #179

suyashb95 commented Oct 27, 2018 •

edited

Loading

suyashb95 commented Oct 27, 2018

suyashb95 commented Oct 28, 2018

vinayak-mehta commented Oct 28, 2018

suyashb95 commented Oct 28, 2018

codecov-io commented Oct 28, 2018 •

edited

Loading

vinayak-mehta Oct 30, 2018

suyashb95 Oct 31, 2018 •

edited

Loading

vinayak-mehta Oct 31, 2018 •

edited

Loading

vinayak-mehta Oct 30, 2018

suyashb95 Oct 31, 2018

vinayak-mehta commented Oct 31, 2018

suyashb95 commented Oct 31, 2018 •

edited

Loading

vinayak-mehta commented Oct 31, 2018

suyashb95 commented Oct 31, 2018

vinayak-mehta commented Nov 1, 2018

vinayak-mehta commented Nov 1, 2018

suyashb95 commented Nov 1, 2018

suyashb95 commented Nov 1, 2018

vinayak-mehta commented Nov 1, 2018

suyashb95 commented Nov 2, 2018

vinayak-mehta commented Nov 2, 2018

vinayak-mehta commented Nov 2, 2018

suyashb95 commented Nov 2, 2018

[MRG + 1] Create a new figure and test each plot type #127 #179

[MRG + 1] Create a new figure and test each plot type #127 #179

Conversation

suyashb95 commented Oct 27, 2018 • edited Loading

suyashb95 commented Oct 27, 2018

suyashb95 commented Oct 28, 2018

vinayak-mehta commented Oct 28, 2018

suyashb95 commented Oct 28, 2018

codecov-io commented Oct 28, 2018 • edited Loading

Codecov Report

vinayak-mehta Oct 30, 2018

Choose a reason for hiding this comment

suyashb95 Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

vinayak-mehta Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

vinayak-mehta Oct 30, 2018

Choose a reason for hiding this comment

suyashb95 Oct 31, 2018

Choose a reason for hiding this comment

vinayak-mehta commented Oct 31, 2018

suyashb95 commented Oct 31, 2018 • edited Loading

vinayak-mehta commented Oct 31, 2018

suyashb95 commented Oct 31, 2018

vinayak-mehta commented Nov 1, 2018

vinayak-mehta commented Nov 1, 2018

suyashb95 commented Nov 1, 2018

suyashb95 commented Nov 1, 2018

vinayak-mehta commented Nov 1, 2018

suyashb95 commented Nov 2, 2018

vinayak-mehta commented Nov 2, 2018

vinayak-mehta commented Nov 2, 2018

suyashb95 commented Nov 2, 2018

suyashb95 commented Oct 27, 2018 •

edited

Loading

codecov-io commented Oct 28, 2018 •

edited

Loading

suyashb95 Oct 31, 2018 •

edited

Loading

vinayak-mehta Oct 31, 2018 •

edited

Loading

suyashb95 commented Oct 31, 2018 •

edited

Loading