You know, mpl is a plotting library, why don't we plot the snapping function to see if it makes sense? Maybe include it as a test so that future changes have to be visually approved?

yes, I have been also thinking about those kind of things. Unfortunately, they do not have high priorities in my TODO list. So, any contributions will be welcomed.

I agree this change is probably worth making. It causes a lot of test failures, so they'll need to be checked and updated. I'm looking at that now...

I don't think we want to make this change -- rounding is actually not what we want -- we want floor-like behavior so that things stay aligned with one another. Look at the alignment of ticks to the axes rectangle with this change.

Okay, I see the problem. I think the primary reason that my fix shows incorrect alignment between axes rectangle and ticks is because numbers are floored in other place (e7172a4). In fact, can you explain why you are using flooring? As far as we use consistent behavior, I don't see why rounding would not work. Maximum error (difference between original value and floored value) in flooring is 0.99999..., while it is 0.5 in rounding.

Here is a little script that demonstrate my issue with flooring.

from matplotlib.transforms import IdentityTransform
trans = IdentityTransform()

plot([200.49, 200.49], [100, 200], "g-",
     snap=True, transform=trans)
plot([200.49, 200.49], [200, 300], "r-",
     snap=False, transform=trans)

If this script is run with the current master branch, the rendered image shows one-pixel offset in x positions of the two vertical lines, although they are given same x-values. And this is because 200.49 became 199.5 (not 200.5) when snapping is used.

@leejjoon -- the snapping should all be happening through the same function. I'm not sure what you're referring to with the flooring -- the link e7172a4 seems to go to something unrelated to this.

One of the problems with rounding vs. flooring is that if you have a rectangle and you round both sides, it can shrink or grow by as much as 2 pixels, whereas with floor the maximum delta is 1 pixel. There are also places where we round the size of the rectangle first, round one of the corners and then set the other side based on that.

@mdboom / @leejjoon : Any update on this? We don't seem to have an agreement on desired behaviour. Do we even have agreement that we want the behaviour to change (should @leejjoon's example be doing what it is)?

It would be good to get this PR moving again.

Thanks,

Going back the the OP on this issue:

When m_snap_value is 0.5, 1.5 and 1.51 is snapped to 1.5, while 1.49 is snapped to 0.5.

This isn't surprising -- pixel centers are on 0.5 in Agg, there has to be a discontinuity somewhere, and on a certain level it's arbitrary where that discontinuity is. However, this fix has the advantage that it puts the discontinuity on the integer boundaries, which is probably the least surprising. So I'm +1 on this, but there are some other places with how markers, text and images are handled that need to be updated before this change produces images that are objectively "better". Expect a PR shortly.

And @WeatherGod: since you asked for it:

And I'm ready to take it back, with a counter example ;)

You can see here that the "rounding" approach warps marker shapes unpleasantly.

Using the approach in this PR:

On master:

And with what I think I'd like to propose (the red line in the plot above):

And the reason for this, I believe, is that given that markers have very meaningful points around the integer transition (i.e. exactly at 0), we actually don't want a discontinuity there, but in fact we want it centered around it.

+1

#1800 was merged, so I'm closing this. Please reopen if I did a bad.

@dmcdougall : Yes, thanks, I think #1800 does fix this.