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

Create a new figure and test each plot type #127

Closed
vinayak-mehta opened this issue Oct 5, 2018 · 13 comments
Closed

Create a new figure and test each plot type #127

vinayak-mehta opened this issue Oct 5, 2018 · 13 comments
Labels
enhancement help wanted moderate

Comments

@vinayak-mehta
Copy link
Contributor

Camelot supports plotting of 5 types of geometries for debugging and tweaking parser configurations.

Plotting needs to be refactored in such a way that all plot functions return a matplotlib figure, so that they can be tested either by using pytest or matplotlib's image_comparison decorator. You can also check how pandas tests plots.

The user should be given an option to save their plots, in addition to the current option of just displaying the figure, which might fail when there are no screens available.

The new plots should also be tested to work in Jupyter notebooks.
@vinayak-mehta vinayak-mehta mentioned this issue Oct 5, 2018
Closed
8 tasks
@vinayak-mehta
Copy link
Contributor Author

Plotting is the only module that isn't being tested right now, this should increase coverage to 90%.

This was referenced Oct 9, 2018
Closed
Closed
@suyashb95
Copy link
Contributor

Does this still need work?

@vinayak-mehta
Copy link
Contributor Author

@suyash458 Yes!

@suyashb95
Copy link
Contributor

Had a look at #150 and #96. Since it's a debugging feature, plot() can be moved from the Table class to the plotting module and I could add some tests against baseline images. The plots can be saved using plt.savefig() instead of displaying them

What do you think?

@vinayak-mehta
Copy link
Contributor Author

@suyash458 What do you think we should use to do this, pytest or matplotlib's image_comparison? +1 on moving plot from Table class to the plotting module. We can call it plot_pdf and add an import to __init__ so that it's available using camelot.plot_pdf. I think plot_pdf should have an option to save or show the plot based on a kwarg.

Yes, baseline image for testing each type of plot should be generated, which can be done with the CLI or the API.

@suyashb95
Copy link
Contributor

suyashb95 commented Oct 23, 2018 •
edited
Loading

@vinayak-mehta pytest-mpl looks like the better option as it's more customizable. The tolerance, baseline directory and file name can be specified explicitly. There are options to generate the baseline images
(using --mpl-generate-path=baseline) and skip image comparison tests as well

A kwarg(defaults to False?) to save the plots optionally sounds good, I guess the filename should be provided by the user? Or should it be generated in plot_pdf?

@vinayak-mehta
Copy link
Contributor Author

@suyash458 pytest-mpl LGTM too. Yes, plot_pdf should have the same behavior that plot has right now, i.e. showing the plot directly while also giving the user an option to save it if a filename is specified.

suyashb95 added a commit to suyashb95/camelot that referenced this issue Oct 27, 2018
@suyashb95
[MRG] Create a new figure and test each plot type atlanhq#127
dbf68a4 
 - 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
Copy link
Contributor

Can't the contour plot be done with a Rectangle patch instead of cv2.rectangle ?

@vinayak-mehta
Copy link
Contributor Author

IIRC, I used cv2.rectangle because it drew a rectangle over the image, which I wasn't able to do with matplotlib patches at the time. Replacing cv2 with matplotlib would be great since it's an unnecessary dependency in plotting.

@suyashb95
Copy link
Contributor

As per this answer, passing fill=None or facecolor='none' to patches.Rectangle() should render an unfilled rectangle over the image

@vinayak-mehta
Copy link
Contributor Author

Looks good, can you try using it in plot_contour?

@suyashb95
Copy link
Contributor

The results are identical, I've updated #179

vinayak-mehta pushed a commit that referenced this issue Nov 2, 2018
@vinayak-mehta
[MRG + 1] Create a new figure and test each plot type #127 (#179)
c0e9235 
* [MRG] Create a new figure and test each plot type #127

 - 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

* Change matplotlib backend for image comparison tests

* Update plotting and tests
 - 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

* Update plot tests and config
 - remove unnecessary asserts
 - update setup.cfg and makefile with `--mpl`

* Add  to

* Add tolerance

* remove text from baseline plots
update plot tests with `remove_text`

* Change method name, update docs and add pep8

* Update docs
@vinayak-mehta
Copy link
Contributor Author

Closed in #179.

@vinayak-mehta vinayak-mehta mentioned this issue Jul 21, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement help wanted moderate
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@vinayak-mehta @suyashb95