@afvincent, this is encouraging. In working through this, have you figured out what "mutation_scale" and "dpi_cor" really are? Can they be explained in the documentation so that a user will be able to understand how to use them? It looks to me like dpi_cor should not be exposed at all--it should be a private internal variable, not a kwarg. It's not used in any of our examples. I'm not saying this is a change that can or should be made in this PR, but maybe we can lay out a strategy for improving the API and documentation.

0 - TL;DR

Somebody corrects me if I am wrong but I would say that:

• dpi_cor is a convenient scaling factor, used only internally to convert between points and pixels and shouldn't be exposed to the user;
• mutation_scale is a reference scale in points, used to compute the size of elements like the head or the tail of an arrow.

For people who have time, here are some explanations.

1 - What is dpi_cor?

Well grep dpi_cor patches.py shows that dpi_cor is only used to do things like:
self.set_dpi_cor(renderer.points_to_pixels(1.)) or dpi_cor = renderer.points_to_pixels(1),
and then:
X = X * dpi_cor or self.set_X(X * dpi_cor),
with X assumed to be in points (for example X is mutation_scale or linewidth).

So I would say that dpi_cor is simply a scaling factor, used to convert the relevant values from points to pixels when drawing the figure. I am not convinced it is very wise to expose it to the user as it is currently done in FancyArrowPatch or ConnectionPatch: the provided value will be overwritten anyway.

2 - What is mutation_scale?

Concerning mutation_scale, it is less clear to me. I would guess it was firstly introduced with the BoxStyle and FancyBboxPatch classes. Indeed their docstrings explain:

An instance of any boxstyle class is an callable object, whose call signature is:
__call__(self, x0, y0, width, height, mutation_size, aspect_ratio=1.)
and returns a :class:Path instance. x0, y0, width and height specify the location and size of the box to be drawn.
mutation_scale determines the overall size of the mutation (by which I mean the transformation of the rectangle to the fancy box).

NB: from reading other method signatures, I would guess that mutation_scale is the same thing as mutation_size (maybe it would be worth to thoroughly check it and change it for more consistent notations among the complete patches.py?)

Then ArrowStyle is defined, mainly for FancyArrowPatch and one can also read in its docstring:

An instance of any arrow style class is a callable object, whose call signature is:
__call__(self, path, mutation_size, linewidth, aspect_ratio=1.)
and it returns a tuple of a :class:Path instance and a boolean value. path is a :class:Path instance along which the arrow will be drawn.
*** mutation_size and aspect_ratio have the same meaning as in :class:BoxStyle. ***

If I understood correctly everything, it is then for example called from inside FancyArrowPatch, when doing:

_path, fillable = self.get_arrowstyle()(_path,
                                        self.get_mutation_scale() * dpi_cor,
                                        self.get_linewidth() * dpi_cor,
                                        self.get_mutation_aspect()
                                       )
# NB: the 2nd line tends to support the idea that mutation_size
# is nothing else than mutation_scale, doesn't it?

FancyArrowPatch docstring explains also:

mutation_scale : a value with which attributes of arrowstyle (e.g., head_length) will be scaled. default=1.

Note that no units are precised. And this description is confirmed by this kind of lines in some methods of ArrowStyle:

head_length = self.head_length * mutation_size

and one can also read:

head_length and head_width determines the size of the arrow relative to the mutation scale.

So I would say that both head_length and head_width are unitless, while mutation_scale has a unit, as we will just just after.

Further, in ConnectionPatch, is it written:

mutation_scale: default is text size (in points).

So mutation_scale does apparently have a unit (the point). (And here its default is not 1 anymore, but text_size, which by the way is arbitrary fixed to 10 (points) if one reads the __init__.)

The case of the class Annotation

FancyArrowPatch is heavily used in Annotation (located in text.py), used for example by plt.annotate if I am correct. Here it is also written

mutation_scale: default is text size (in points)

as in ConnectionPatch. However this time, mutation_scale is indeed sized as the text, through lines like:

ms = d.pop("mutation_scale", self.get_size())
self.arrow_patch.set_mutation_scale(ms)

that pass argument to the ad-hoc FancyArrowPatch instance.

3 - Tentative of conclusion about mutation_scale

So from what I read and understood, I see mutation_scale as a reference scale, expressed in points, that is mainly used to compute the dimensions of the head and tail of the arrow when drawing the figure. I will try to see if one can make the docstring a bit clearer about it.

Small remark: from my point of view, the fact that in raw FancyArrowPatch the default value is 1 (pt ?), while in derived classes like ConnectionPatch or Annotation it is much bigger (10 pt or fontsize), may be a bit disturbing for users (at least it is for me ^^). Especially when plotting arrow-only annotations: I get significantly different behaviors (when relying on default values), eventhough I would expect similar results (but maybe it is simply because I consider Annotation as ~ FancyArrow + Text).

About the commit 1df0f7e

Failures in test_axes

I think it solves some of the problems that I didn't expected when patching FancyArrowPatch last time. It should mainly correct the fact that test_polar_annotations and test_polar_coord_annotations were failing. Well they still do, but I am rather convinced it is now essentially due to a problem of tolerance on these two unit tests: reported RMS are rather low, something like 0.003 and when I look at the *-failed-diff.png, it seems uniformly black to me. See the example:

Or am I missing something? By the way, the arrow now behaves correctly :) (it had a different size before with the previous commit).

Failures in test_streamplot

Additionally, I finished to convinced myself that the unit tests test_colormap, test_linewidth and test_masks_and_nans (in test_streamplot) are failing more or less because of the former wrong dpi-dependent behavior of FancyArrowPatch. It is rather clear on the following picture that the exported streamplot depends on the DPI value with the vanilla version of FancyPatchArrow (right panels) while it does not with the patched version (left panels):

Currently the problem is that the two upper panels (@ 100 DPI) are not perfectly identical: the arrow sizes are not the same. It is confirmed by the *-failed-diff.png too. Eventhough the former reference behavior might not be that good anyway (given its results for a higher value of DPI), I will investigate why the arrow size changes with the new FancyArrowPatch.

Failure in test_transforms

From the *-failed-diff.png, the problem with test_pre_transform_plotting is related to a portion of the image with a streamplot, so same “problem” as before.

I do not have time to say anything technically meaningful, but thank you for your detailed work on this!

======================================================================
ERROR: matplotlib.tests.test_artist.test_clipping.test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 55, in failer
    result = f(*args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 249, in do_test
    self._tol, in_decorator=True)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 332, in compare_images
    rms = calculate_rms(expectedImage, actualImage)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 246, in calculate_rms
    abs_diff_image = abs(expectedImage - actualImage)
ValueError: operands could not be broadcast together with shapes (540,720,3) (270,360,3) 

Those failures worry me a bit.

I suspect that if you track down the issue with the stream plots, the polar plot issue will go away.

The new stream plot does look a bit better to my eyes, it would not take much to convince me that the test images should be updated.

It is weird, I have no problem with test_artist on my machine (CentOS 7, Python 2.7 in a conda env, with Matplotlib 1.5.1 + my last commit):

In [3]: run test_artist.py
/home/adrien/matplotlib-git/matplotlib/lib/matplotlib/__init__.py:885: 
UserWarning: axes.color_cycle is deprecated and replaced with axes.prop_cycle; 
please use the latter.  # I still didn't have a look into the new color_cycle thing, 
                        # but that should irrelevant for our concern, shouldn't it? 
warnings.warn(self.msg_depr % (key, alt_key))
/home/adrien/matplotlib-git/matplotlib/lib/matplotlib/testing/decorators.py:363: 
UserWarning: test module run as script. guessing baseline image locations 
warnings.warn('test module run as script. guessing baseline image locations')
.........
----------------------------------------------------------------------
Ran 9 tests in 2.641s

OK

Travis is running this PR against latest master, not 1.5.1. There have been many changes since then, but the most likely one is a change in the default DPI.

Just to say that I didn't have time lately to dedicate checking the final details of this PR, but I am still working on it to understand why some unit tests fail, especially the ones that do by only a few pixels.

About the failures of test_axes

Both test_polar_annotations and test_polar_coord_annotations in test_axes are still failing. I made manual comparisons (with scipy.misc.imread and Co…) and the baseline images and the result images are 100% identical for the SVG and PDF versions (*_svg.png and *_pdf.png). The problem seems to only occur with the PNG export, where 4 and 12 pixels are indicated as not identical between baseline and result. However, I am not sure it is really a problem. Here is a picture where I highlighted the region with the-- only 4 in this case-- problematic pixels (the zoom factor is x4):

Without ImageMagick, one can hardly notice the difference of the 4 pixels… (Here are the 3 inset pictures for those who would like to compare by themselve: ROI_comparisons.tar.gz)

I am wondering how relevant are these two failures. Are such tiny differences something usual ? Plus the fact it only affects the PNG export, and not the SVG and the PDF ones ?

About the other failures

I am still investigating the case of the streamplot failures, but I am rather confident it is only due to the former dpi-dependent behavior. Besides, I expect that solving that will also solve the problem with test_transforms (that seems very similar).

Thanks for the detailed investigation. I tend to agree with you that this change is not significant and we can just replace the image with the new version. It's really hard to set the tolerance for image comparison tests. We used to have a much higher tolerance but that resulted in a number of issues going unnoticed over the years.

@afvincent Echoing Jens, thank you for the very detailed work tracking this down! For changes that small please replace the images.

👍 I am sold on this once the tests are updated.

@tacaswell I did another rebase, skipping the failing 3 last patches (2110e5e, 56f337f and 83fb8d1) about failing PNG baseline test images, and then re-updated these test images on my local repository. Unfortunately, git push origin patch-issue-6035 gives me the following error message (I apologize, my OS locale is French):

 ! [rejected]        patch-issue-6035 -> patch-issue-6035 (non-fast-forward)
error: impossible de pousser des références vers 'git@github.com:afvincent/matplotlib.git'
astuce: Les mises à jour ont été rejetées car la pointe de la branche courante est derrière
astuce: son homologue distant.

Homemade translation:

 ! [rejected]        patch-issue-6035 -> patch-issue-6035 (non-fast-forward)
error: impossible to push references toward 'git@github.com:afvincent/matplotlib.git'
tip: updates were rejected because current branch head is behind the distant head

So I guess I managed once more to get git in an unwanted situation… If it matters, just before rebasing, I used

git checkout master
git fetch upstream
git merge --ff-only upstream/master

Wouldn't it be easier to create a new branch from the current master, on which I will report the changes of the current patch, and then to open a new PR?

PS : I also did git clean -xfdn, but then I had issues with some tests scripts, failing when trying to load matplotlib._path for example. Re-running python setup.py develop in my python virtual environment made it work again. However I still have a lot of failing tests, due to what seems to be tiny differences of fonts (but these failures don't seems to occur on Travis & Co)

You need to do

git push --force origin patch-issue-6035

To tell git to do something that might lose commits.

On Sat, May 14, 2016, 05:41 afvincent notifications@github.com wrote:

@tacaswell https://github.com/tacaswell I did another rebase, skipping
the failing 3 last patches (2110e5e
2110e5e,
56f337f
56f337f
and 83fb8d1
83fb8d1)
about failing PNG baseline test images, and then re-updated these test
images on my local repository. Unfortunately, git push origin
patch-issue-6035 gives me the following error message (I apologize, my OS
locale is French):

! [rejected] patch-issue-6035 -> patch-issue-6035 (non-fast-forward)
error: impossible de pousser des références vers 'git@github.com:afvincent/matplotlib.git'
astuce: Les mises à jour ont été rejetées car la pointe de la branche courante est derrière
astuce: son homologue distant.

Homemade translation:

! [rejected] patch-issue-6035 -> patch-issue-6035 (non-fast-forward)
error: impossible to push references toward 'git@github.com:afvincent/matplotlib.git'
tip: updates were rejected because current branch head is behind the distant head

So I guess I managed once more to get git in an unwanted situation… If it
matters, just before rebasing, I used

git checkout master
git fetch upstream
git merge --ff-only upstream/master

Wouldn't it be easier to create a new branch from the current master, on
which I will report the changes of the current patch, and then to open a
new PR?

PS : I also did git clean -xfdn, but then I had issues with some tests
scripts, failing when trying to load matplotlib._path for example.
Re-running python setup.py develop in my python virtual environment made
it work again. However I still have a lot of failing tests, due to what
seems to be tiny differences of fonts (but these failures don't seems to
occur on Travis & Co)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6054 (comment)

The previous updated PNGs test images seemed to be still failing due to difference in the fonts of the axis labels, so I just re-commited the same images but produced on a computer than seemed to be free of the font issues I have on my personal computer. Apparently the associated failures have disappeared after this last commit.

So now only the failures of test_artist.test_clipping still remain...

I am a bit lost with the failures of test_clipping… After a look at the code of this test, I do not see any method or other stuff I might have directly modified, but yet Travis complains:

======================================================================
FAIL: matplotlib.tests.test_artist.test_clipping.test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 55, in failer
    result = f(*args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 255, in do_test
    self._tol, in_decorator=True)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 337, in compare_images
    rms = calculate_rms(expectedImage, actualImage)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 248, in calculate_rms
    "actual size {1}".format(expectedImage.shape, actualImage.shape))
ImageComparisonFailure: image sizes do not match expected size: (600, 800, 3) actual size (300, 400, 3)

======================================================================
FAIL: matplotlib.tests.test_artist.test_clipping.test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 55, in failer
    result = f(*args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 255, in do_test
    self._tol, in_decorator=True)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 337, in compare_images
    rms = calculate_rms(expectedImage, actualImage)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 248, in calculate_rms
    "actual size {1}".format(expectedImage.shape, actualImage.shape))
ImageComparisonFailure: image sizes do not match expected size: (432, 576, 3) actual size (216, 288, 3)

======================================================================
FAIL: matplotlib.tests.test_artist.test_clipping.test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 55, in failer
    result = f(*args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/decorators.py", line 255, in do_test
    self._tol, in_decorator=True)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 337, in compare_images
    rms = calculate_rms(expectedImage, actualImage)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/testing/compare.py", line 248, in calculate_rms
    "actual size {1}".format(expectedImage.shape, actualImage.shape))
ImageComparisonFailure: image sizes do not match expected size: (540, 720, 3) actual size (270, 360, 3)

Looks like (at least) one of the problems is that the DPI value use when saving the images is half the expected one. QuLogic suggested a while ago it might be related to a change in the default DPI value, but then should'nt there be a lot more of test failures (basically all the image comparison tests)? Furthermore, isn't it a bit strange that even the PNG conversions of the PDF and SVG images are also saved with half the expected resolution?

@jenshnielsen @tacaswell

On a different topic than the remaining issues with test_artist.test_clipping I am still struggling with: axis_grid from mpl_toolkits uses FancyArrowPatch for some inheritance (at least for setting an arrow axis style as in demo_axisline_style). I tested rapidly : indeed the size of the arrow heads of the axes was dpi-dependent before the patch, which is corrected after it. This change was unnoticed before because none of the mpl_toolkits tests seems to make use of such an “arrow axis”. How is this a problem? And should a dedicated test be written?

If you feel compelled to write a test we will not protest, but a vast majority of mpl_toolkit is not tested and cleaning that up is not a burden we want to put on this PR. It is also not very well documented which means that it's usage is probably pretty low.

It also sounds like you have made things better not worse?

I would say the situation is better now, as the size of the arrow heads of the axes are not dpi-dependent anymore. Before the patch, demo_axisline_style produced axes with barely visible arrow head when saved with 300 DPI… But as with the rest of this PR, the arrows produced by default for the axes are now a bit bigger than before (as for example as with test_streamplot).

The issue with test_artist.test_clipping is caused by it executing right after your new tests.

For reasons unknown prepare_fancyarrow_dpi_cor_test is getting executed as a test even if it's not called test_ and since it's not marked as @image_comparison or @cleanup the style (mainly change to dpi I think) is leaked to the next test.

Making the method private by renaming it to __prepare_fancyarrow_dpi_cor_test seems to fix the issue.

Replaced by #6504.