Ensure valid path mangling for ContourLabeler #27045
Conversation
I'd say go with a smoke test at least. Who knows when the "correct" solution shows up... |
|
Test added, I chose a slightly different presentation where instead of changing the dpi, I changed the inline_spacing argument, because that felt like a more direct way of triggering the behavior (I did check that the test failed on Also adjusted to use calculated x/y/z values instead of reading from text files. Here is the image the test code would produce (though it is not an image test, just a smoke test):
The main difference between proposals from #26971 is what would happen to the yellow 0.8 contour. My first proposal would have drawn the curve there, @anntzer's proposal would have not drawn a line at all (but kept the label), and this PR/the last proposal retains mpl 3.7 behavior where a single line segment is drawn. |
Closes matplotlib#27062 The code for rotation had referred to the original path, when it should only be rotating the connected segment, which had already been extracted.
|
I have added a commit which fixes #27062, which is editing the same method, though is a different bug presentation |
|
Nudge for review |
|
Well, in implementing the test I discovered that our test mechanism for enabling the fallback of using collections only ever triggers after calling clabel and the behavior is different between the two... So sorting that out now... |
|
Okay, what was happening was that it was "invalidating" the old style split collections, but not removing them from being drawn (or turning the new style ones that it is manipulating visible again) I guess there is some worry that if people were modifying the old-style paths that those changes are lost, but that seems to be intended by the implementation, this just completes the task. |
|
Do we need all 3 formats for the test? |
6401e08 to
5cf3da9
Compare
|
Updated to remove the additional formats (I had just copied the decorator from a similar test) I also removed the tolerance, though depending on why it was added to the previous test, it may need to be added back. |
…045-on-v3.8.x Backport PR #27045 on branch v3.8.x (Ensure valid path mangling for ContourLabeler)
PR summary
Closes #26971
Closes #27062
As noted in the comment and the discussion on the linked issue, this is actually technically "wrong", but is provided as a stopgap so that a more proper fix can be made.
In particular, the original sin here is, in my opinion, that the padding is specified in pixels, rather than either a scale-independent metric (e.g. points) or a value relative to the fontsize.
Using pixels especially does not make sense when saving to vector based formats. (As is actually done in the issue as reported)
Changing that behavior, however is a much larger api change, especially since this should be backported to the 3.8 series.
As it stands, the original calculation would actually indicate that the entire contour should be eliminated, but that leaves a label with no contour, which I also think is undesireable.
Perhaps as a slightly larger change it could e.g. return
angle=np.nan,<the path as input>to indicate that no label should be added and keep the contour.That much is a larger behavior change than I really want to introduce here (as I intend this to be backported)
Current
mainbehavior retains the two end points but then only has one code, so it produces an invalid PATH and fails.This PR ends up drawing a single line segment between the two identified endpoints, and ignores any points that may be between them.
But this appears to be the behavior of mpl 3.7 anyway, and is a relatively rare edgecase, brought on by either really large padding or really low DPI (with the test case in the issue it was 4 dpi, and even 5 dpi avoided problems).
So personally, I'm okay with just minimally avoiding invalid paths being generated and not worrying about too much more than that for the initial fix.
I could add a test case, but I think that may be better as part of moving towards a non-dpi dependent padding.
PR checklist