Repository navigation
Conversation
…rison and unit test case
ghost
changed the title
dstansby
left a comment
dstansby
left a comment
There was a problem hiding this comment.
A couple of small changes/questions, but overall this looks good.
| plt.subplot() | ||
| plt.axis('off') | ||
| plt.plot(np.sin(np.linspace(-5, 5, 100)), 'v', c='none') | ||
| try: |
There was a problem hiding this comment.
I think this try/except block can go; if the next line throws and exception the test will automatically fail.
| plt.savefig("figure.eps", format='eps') | ||
| result = compare_images('figure.pdf', 'figure.eps', 0) | ||
| assert result is None | ||
| from os import remove |
There was a problem hiding this comment.
The following line can just be os.remove('figure.eps'), and then this import line isn't needed.
There was a problem hiding this comment.
These files should be saved in a temporary directory (use the (tmpdir fixture), and you won't need to delete them.
There was a problem hiding this comment.
But also, I thought @tacaswell said this wasn't necessary.
There was a problem hiding this comment.
Either way on this staying or not, but 👍 to using the tempdir fixture.
There was a problem hiding this comment.
ah, tempdir fixture sounds cool, I didn't know what it was before. Will fix this then. Resubmit tomorrow.
|
|
||
| def test_pdf_savefig_when_color_is_none(): | ||
| backup_params = mpl.rcParams.copy() | ||
| mpl.rcParams.update(mpl.rcParamsDefault) |
There was a problem hiding this comment.
Is there a particular reason why the rcParams have to be updated to the defaults?
There was a problem hiding this comment.
I will have to get back on this one when I'm back from work. It was two days ago, can't recall the reason on the spot.
There was a problem hiding this comment.
@dstansby The reason for using default rcParams is that in testing environment, rcParams['_internal.classic_mode'] == True and the resulting code path of this setting does not go through the error i.e. gc._rgb is always not None. In other words, this error (and the required fix) is very specific to the setting where rcParams['_internal.classic_mode'] == False, which happens to be the default behaviour. Admittedly, I over-killed the setting by set everything back to default rather than a single setting.
There was a problem hiding this comment.
All settings are reset at the beginning of tests; use @pytest.mark.style('default') to change styles.
|
@dstansby @QuLogic @tacaswell some codecov test fails, but given this is a very simple fix, I wonder if these failures can be overlooked. |
| fig.savefig(tmpfile, format='pdf') | ||
|
|
||
|
|
||
| @pytest.fixture(scope='function') |
There was a problem hiding this comment.
Did you mean to make this a new fixture? The coverage is failing because this is never getting run
|
@Tuan333 I am going to push a commit to your branch removing the |
|
Actually it seems @Tuan333 has deleted their Matplotlib fork, going to grab these commits, add the extra commit, push to my fork and make a new PR. |
|
Closing as commits are moved to #8850 |
QuLogic
modified the milestones:
2.1 (next point release),
2.0.3 (next bug fix release)
PR Summary
The second pull request to address #8142 to address @tacaswell 's comment in #8644. Added a test case to compare generated figures using pdf backend with eps backend (the later does not have this bug). Added some small fix to testing facility in the process. Note that the current pdf to eps comparison function is probably only worthwhile when comparing blank/non-blank images
PR Checklist