This makes sense (as in no harm in adding it and useful when needed).

Two things:

1. The reason that the tests are failing is that pyplot is not updated, please run python tools/boilerplate.py to generate a version with the new arguments.
2. Please add a note in doc/users/next_whats_new (using the template) which simply states that these are added. I think it would be nice for users to know this as others may have similar need to control it.

One may consider adding a bit more description about the effects of changing the value. Although, it may be obvious once you're into it, it may take a bit of thinking to figure out what a larger or smaller value will lead to. Maybe just a simple statement as "For a value larger than 1, the streamline computation will be faster, but less accurate." or something (if that now is correct...). Can go in the note or doc-string.

Thanks for the helpful review, @oscargus! Hopefully these changes should cover all the bases.

@jklymak I’ve now added an example and associated test showing the results from varying the new parameters.

I’m not sure the best way to handle the failing PR cleanliness test.

Also, will these changes have to wait until the next minor release or will they be in the next patch release? I’m wondering what to put in the version added directive.

Looks good. I'd include the default, and maybe consider showing the analytical result?

Sounds good! I added the default and timings. I could add the analytical result (contours of the streamfunction), but it's essentially identical to the streamlines with the smallest steps and might make the code more complicated to understand.

Well my secret reason for suggesting it was that many folks seem to want a super precise streamlines when they have a non-divergent field that they could more precisely represent by contouring the streamfunction. Indeed contouring the streamfunction is superior in many cases because the distance between evenly spaced contours is proportional to the flow speed, a property the streamline method can only approximate if the upstream flow is constant.

Ah I see. Since it'd complicate the code, there would be limited visual difference, and it's a little bit of a tangent for this case (though certainly important for streamlines of analytic functions), I'm inclined to skip it for this example. But if you feel strongly about it, I can add it.

Nope, fair enough. If I fell strongly enough about it, I'll write a follow up PR. This looks great as-is. Thanks!

Great, makes sense. Thanks for the review!

I am in general 👍 on this change and the test and docs look good.

However I think the parameter names are very confusing and should make clearer that these are scales not absolute numbers.

For the clenliness test, it looks like you tried to do a rebase and then merged the original branch back in. I can try to clean it up for you, but if you would like to try yourself:

git rebase upstream/main       # or whatever you named the upstream remote to matpotlib/matplotlib
git push --force-with-lease    # maybe with a branch name 

@tacaswell is your comment about the parameter names before or after my latest commit?

I gave the git fix a shot, but it is giving me errors locally. I think there was also a cleanliness complaint that I added and edited the new pdf to test against in the same PR, which I also don't know the best way to fix. If you have quick changes that you know would solve these, it'd be super if you're willing to make the edits. If not, I can look into it.

@tacaswell is your comment about the parameter names before or after my latest commit?

typed before, posted after 🤣

I think there was also a cleanliness complaint that I added and edited the new pdf to test against in the same PR, which I also don't know the best way to fix

squash it all to one commit, see https://matplotlib.org/devdocs/devel/development_workflow.html#rewrite-commit-history

I think I've addressed all the comments now! We'll see if the tests pass with an image comparison tolerance of 0. Thanks again for the reviews

OK, so full-circling on this; do these parameters actually matter? It seems by allowing max_step_size >1 we are giving the user a foot cannon because the integration exceeds the CFL criterion. For max_step_size < 1 does it really help? The interpolation is linear, and stopping inside a grid cell and checking it a bunch of times is not likely to change the resulting vector.

If there are situations where this helps, it would be useful to put one in the example. It is also worth considering not allowing max_step_size>1,

Well this change was motivated because my coworker and I have a CFD solution (which is obviously too big to be included in the examples) where we were plotting streamlines of characteristics rather than velocities. With the default step size, the streamlines do not accurately stop at the edge of the supersonic region. In the images, you can see how the larger step size looks somewhat quantized and the streamline placement is much less precise.

Default error and step size:

100x tighter error and step size:

Another motivation is that we have seen cases where streamlines that go close to the geometry "jump" into the undefined geometry region rather than picking up the boundary layer and following the fluid domain solution around the shape. There is so much resolution in meshes in the boundary layer that linear interpolation within a cell is not a problem.

Finally, others online seem to have found use in smaller step sizes. However, they had to edit the source code because it's a hardcoded value and the parameters are not exposed.

I'm open to limiting step size and error scaling parameters to <= 1. I just figured it wouldn't hurt to keep this general in case there are other scenarios where it's useful to take bigger steps. I kind of assumed that if users are getting to the point of even looking for these options, they might have some idea of what they'd like to see (and if they change it and it looks bad, they'd change it back).

Well if the issue is lots of curvature could you use potential flow around an ellipse transverse to the flow instead of a circle? I mean it's probably fine to just include this and folks who need it can use it, but it would be preferable to show its utility.

I tried the transverse ellipse but it didn't show the intended behavior where the defaults go through it (though the coarsest step version did). But I have an alternate proposal. If you seed streamlines inside the circle (of radius 1), they should stay inside the circle. Here I seed points at x = 0 (center) from y = -0.999 to y = 0.999. The only case where they stay inside is with the smallest step size and tightest error. I checked that this isn't caused by interpolating a coarse grid because the result is the same even if I change the 50 x 50 grid of velocities to 500 x 500 (perhaps it requires even finer, but I think it's the step/error as intended). Is this preferred to the original example?

To be honest, part of the benefit of a smaller step size is to making the streamlines very smooth. In the original example, you can observe jaggedness in the streamlines with the default settings (look closely near the stagnation point). This alone would be enough for me to want a smaller step size for a publication figure if it costs me only a few seconds extra of computation. I think the original example shows this feature better than this newly-proposed one (maybe I could zoom in on that region in the original example?).

Sure, if you can zoom, that would really help show why this helps. Agree that if it's a subtle effect it may still be important.

Sure thing, I’ll add a zoom box!

I see the mypy tests failing now, but it doesn't look like it references any changes made in this PR. Did I do something wrong to cause it to fail?

It is unrelated: #29362

Thanks for your detailed review, definitely improved the addition!

Thanks again @eytanadler - appreciate your patience with the process, which can take a while.