I guess what is happening is that you need to do the simplification in screen units, but the later draw routine needs the path in data units, so you need to convert one extra time back and forth? That seems inefficient, any chance you could either 1. do the simplification at the last minute (once you don't need to convert back and forth anymore), or at least 2. keep the simplified path in screen units, and set the transform to identity?

Also, instead of prints (which need to go anyways :-)), some tests (e.g. with paths for which you know what the result of simplification should be) would be welcome, to show that the simplifier is indeed working as expected.

Copying a comment that I haven't pushed yet:

# the path simplification code expects the path
# to be in pixel space, not data space, but the
# renderer downstream expects the path to be
# in data space. The transformation `affine`
# will put the path into pixel space, and
# affine.inverted() will transform it back to
# data space. So, transform using affine, simplify
# the path, and transform using the affine inverted.

It is inefficient, but I see no way around it at the moment. Do you know what populates the renderer argument? The only way to delay cleaning until it has been transformed is to do it inside this call: renderer.draw_path(gc, path, trans) from

    def _draw_solid(self, renderer, gc, path, trans):
        gc.set_linestyle('solid')
        gc.set_dashes(self._dashOffset, self._dashSeq)
        renderer.draw_path(gc, path, trans)

Obviously print statements will be removed and tests will be added.

Testing in general will be a huge pain, since nearly every single image will fail the current image comparisons. We may have to change the default value of path.simplify to False to avoid this...

Yeah, it looks like renderer is basically the backend. Without requiring changes to the backend API (so that it takes in an already transformed path, and I'm sure it was not designed that way initially also for performance reasons), I don't think we can avoid it. It's still faster than not doing the downsampling.

You may be able to get away with editing _classic_test.mplstyle, which is basically there for that reason (we try not to update all test images at once if possible, as this really leads to bloating of the repo size).

@anntzer are you saying if I edit _classic_test.mplstyle to turn path simplification off, that all the figures created during testing will be made with simplification turned off?

Do you know if that behavior is documented anywhere? I've been reading through developer documentation but not finding anything.

I hope so, at least...

would something like

tpath = affine.transform_path(tpath).cleaned(simplify=True)
affine = IdentityTransform() # or whatever it is called

not work to avoid the back and forth discussed earlier?

I'll play around with that. I'm a little worried about changing that since I don't know what downstream rendering code relies on (does it use the data-space to generate axes labels? scale text? etc.). It might be premature optimization at this point.

I really meant something like "Check if affine is used anywhere after. If not, do /previous message/. If it is, set a flag somewhere, and only use an identity affine when drawing the path".
Well perhaps the second one would be an premature optimization if needed, but please give the first one a try if possible, given that that would be fairly simple.

Hmm. Initial tests just created a blank figure when doing that.

Sorry didn't see your 2nd message before posting. So I guess it looks like affine is being used for something else later on, I'm guessing in the clipping routine:

<path_converters.h>
/*
 This file contains a number of vertex converters that modify
 paths. They all work as iterators, where the output is generated
 on-the-fly, and don't require a copy of the full data.

 Each class represents a discrete step in a "path-cleansing" pipeline.
 They are currently applied in the following order in the Agg backend:

   ...

   3. PathClipper: Clips line segments to a given rectangle.  This is
      helpful for data reduction, and also to avoid a limitation in
      Agg where coordinates can not be larger than 24-bit signed
      integers.

   ...

   5. PathSimplifier: Removes line segments from highly dense paths
      that would not have an impact on their appearance.  Speeds up
      rendering and reduces file sizes.

   ...
 */

Sad, but I can live with that. Perhaps also mention the issue in a comment?

Added a comment. It's possible that there may yet be a simple path forward (pun intended) on this. I'll take a look tomorrow with fresh eyes.

the test on simplify_threshold should go above (say just after path.simplify), in order to avoid iterating on the codes if possible? (yes, I realize that's unlikely to be the bottleneck)

also it looks like we may be able to deprecate path.simplify and fold it into path.simplify_threshold being zero or not.

Good idea. I think combining the parameters makes a lot of sense. They are definitely redundant in purpose. I'll probably also suggest upping the default from 1/9 to something closer to 1/2. It turns out the simplify_threshold is squared in the simplification code, so 1/9 is actually only reducing paths that are 1/81 pixel away from being not parallel! Not sure what the original logic on that was. Perhaps original author was thinking of it in terms of square pixels difference...?

Perhaps that can go into a separate PR (linked to this discussion)? If you look around mpl/init.py and mpl/rcsetup.py you should see how rcparam deprecation is handled. That'll need a notice in api_changes too (this PR will probably benefit from an entry in the docs whatsnew too).

this PR will probably benefit from an entry in the docs whatsnew too

Yep, already have a draft in doc/users/whats_new/path_simplification_updates.rst

Realized this morning that it looks like it's squared in simplification code to avoid having to compute sqrt step of norm calculation, so it should be behaving as advertised.

Perhaps just remove the comment?
You may also be able to just use a module-level autouse fixture to set the rcparam (see https://docs.pytest.org/en/latest/fixture.html).

thanks for the tip!

Is there a reason not to make this autoused? (perhaps, I'm asking)

There is exactly one test that turns it off, but it's turned off manually in the test... so no, apart from me not reading pytest docs thoroughly :)

Hmm. In fact, using scope='module' makes some tests fail. Possibly b/c it's only run once in this module, meaning the test that turns off simplification would effect later tests? Don't know for sure. I'll leave it as is for now.

Thanks for all your help so far @anntzer

If the comment is out of date, just remove it or update it accordingly.

Looking at it, I don't think it needs to be a full changelog of that code chunk, it can just describe what it's doing and mention the critical improvements, or ideas that have been tried and did or did not work.

should m_lastBackwardMax be reset here as well?

I'm pretty sure it can only be false at this point in the code. It wouldn't hurt though, so if you're worried about it I'm OK adding that in.

It is also helpful breadcrumbs for the next person to look at this code.

added in latest commit

Can you add a remark here about reasonable values and why you would want to use this?

As near as I can tell this feature is un-documented except for a comment in the change log.

There was some discussion as to folding path.simplify and path.simplify_threshold into a single rcparam (zero vs nonzero) that got lost in the thread, too. @kbrose Do you plan to do it? It can be a separate PR of course.

I'm adding some more text for the whats new entry now. Do you think it's worth adding more (permanent) documentation somewhere?

Yeah I think it would be cleaner as a separate PR (I'm willing to do it, but I'll be focusing on this and other performance optimizations first). @anntzer @tacaswell the idea was that path.simplify could be removed because it is redundant with path.simplify_threshold == 0.

No hurry.
Perhaps start a "performance" section in http://matplotlib.org/faq/index.html or http://matplotlib.org/faq/usage_faq.html?

I liked that idea, but usage faq has been removed from master? Looks like it happened in eaaa2b4. Guess I'll put somewhere else

It probably got moved to 'tutorials/01_introductory/usage.py' but github is being too lazy about the diffs.

👍

This is true at least for the given offsets and current seed:

>>> np.random.seed(19680801)
>>> np.random.rand(15) - 0.5
array([ 0.2003673 ,  0.24275081,  0.20928001,  0.06674552,  0.47778533,
        0.20633485, -0.25208424, -0.34211665,  0.19769852,  0.21995667,
       -0.24225557, -0.15845322,  0.46876117,  0.1945071 , -0.03361674])

Yep - I put that comment in as a warning to future dabblers

Please add space after commas.

This link might get changed by #8975, so need to be wary of whichever gets merged first...

Note: Current version of that PR would change it to sphx_glr_tutorials_introductory_customizing.py

This setting is ridiculously fast!

This ref will be has been outdated by #8983.

What's the new ref?

sphx_glr_gallery_lines_bars_and_markers_markevery_demo.py ?

Yea, that looks right.

This link might get changed by #8975, so need to be wary of whichever gets merged first...

Note: Current version of that PR would change it to sphx_glr_tutorials_introductory_customizing.py