Still another Agg snapping issue. #800

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

leejjoon commented Mar 27, 2012

 With the current code, 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.
 leejjoon `improve snapping in agg backend` `05e3e28`
Member

WeatherGod commented Mar 30, 2012

 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?
Contributor

leejjoon commented Mar 30, 2012

 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.
Owner

mdboom commented Apr 13, 2012

 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...
Owner

mdboom commented Apr 13, 2012

 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.
Contributor

leejjoon commented Apr 14, 2012

 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.
Owner

mdboom commented May 14, 2012

 @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.
Member

pelson commented Nov 3, 2012

 @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,

Closed

Owner

mdboom commented Mar 1, 2013

 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:
Owner

mdboom commented Mar 1, 2013

 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):

mdboom added a commit to mdboom/matplotlib that referenced this pull request Mar 1, 2013

 mdboom `Fix a number of snapping issues. Incorporates ideas from #800 (see th…` `…ere for further discussion) and #1591. This, unlike #800, doesn't change the snapping in a fundamental way, but it does help with text/image/marker alignment of snapped and non-snapped objects.` `c48afe4`
Owner

mdboom commented Mar 1, 2013

 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.

Merged

Member

 +1
Member

dmcdougall commented Mar 23, 2013

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

Owner

mdboom commented Mar 25, 2013

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