I would suggest starting with sticking to the mpl2014 algorithm for now so that we don't have to update the images? Actually switching the algorithm (perhaps configurable globally via an rcParam?) could be done as a second step?

Do you know what happened to patheffect2? The change isn't a huge deal, but the old "18" label position seems better?

Do you know what happened to patheffect2? The change isn't a huge deal, but the old "18" label position seems better?

I do, and it is most unfortunate.

The test_patheffects2 images are different as it uses contour to generate some lines to put the path effects on. There is a change I purposefully put in contourpy's mpl2014 algorithm that is not in mpl's version of it. Mpl's doesn't allow identical adjacent (x,y) points in a contour line/polygon, but contourpy does. This is because some rendering/processing libraries that consume polygon data very sensibly only allow polygons that contain at least 3 points. So the 2 versions produce the same visual output in Mpl but the underlying point arrays from contourpy may be longer with adjacent duplicate points.

Mpl's label positioning algorithm evidently uses the number of points in a line to work out where to put the label, so it can be different now. If the contour lines weren't going exactly through the grid points there would be no difference.

The minimum versions CI test is failing because in contourpy I have pinned a minimum numpy version of 1.20 whereas it is 1.19 in mpl. It doesn't really need to be 1.20 so I can put it back to 1.19, but that will need a new release of contourpy.

Actually switching the algorithm (perhaps configurable globally via an rcParam?) could be done as a second step?

I am not sure about the idea of using a new rcParam. In the long run we will use the new serial algorithm as that is the only way we will get the improvements in functionality and performance. When that happens, do we deprecate the new rcParam or add documentation to explain the available options? Maybe it would be better to make the decision for end users so that they don't have to learn about the alternatives. But I happy to be guided by others on this.

We have lots of machinery for deprecating rcParams, cf. #13761, #15318, #17156, but I don't know whether it's the best choice to use one here.

We probably need to agree a strategy going forward here. Do we run with this as it is (current algorithm) for one mpl release and then switch to the new algorithm for the next mpl release to spread the risk, or do we do both changes in one go?

As discussed on the call:

• lets use the 2014 algorithm for 3.6
• having a kwarg is great for the new algorithm, so long as it has a name that is not "new"/"old".
• can start doing the dance to whatever default we prefer for 3.7

From my quick look I think this still needs docs, a change note, and the new kwarg if you are planning to offer it.

I wish you had given me some notice that you were going to talk about this on the call so that I could attend and give an informed opinion. The algorithms already have names (https://contourpy.readthedocs.io/en/v1.0.1/user_guide/name.html) and the names in question here are "mpl2014" and "serial". Obviously they wouldn't be called "old" and "new".

I still need some clarity here. Sticking with "mpl2014" for Matplotlib 3.6 is fine, in which case you don't need a kwarg until after 3.6. Or do you mean you'd like a kwarg now but the default would remain as "mpl2014"? The logical progression from this is that you need to include some tests for both possible kwargs (or indeed all 4 that ContourPy supports) and hence you are going to have to include two (or possibly 4?!) sets of baseline images.

@ianthomas23 Sorry about that, will you be able to make the call next week?

The analogy we had was the interpolation kwarg on imshow. My position is that we should not block this PR on getting a kwarg is place for 3.6. If we get a kwarg (that defaults to 'mpl2014') on for 3.6 👍🏻 , but we should also bank the win of depending on contourpy as-is!

If there are 4 algorithms I would go with something like https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/tests/baseline_images/test_image/imshow_masked_interpolation.png to make sure they all run and leave a majority of our test images using 'mpl2014'. We might even be able to reduce some of our contour tests as I am happy to trust that the testing on contourpy is doing a more complete job of testing the algorithms than we would want to do.

@ianthomas23 Sorry about that, will you be able to make the call next week?

Yes, I can make this week's call.

Thanks for the clarification, it makes more sense now. Getting this all done for 3.6 should be fine. To do list (mostly for my own benefit):

1. Add kwarg for algorithm name. I am using e.g. name='mpl2014' in contourpy. For mpl using name=... is maybe not precise enough but the clearer algorithm=... or algo=... are possibly worse choices. Maybe name=... is OK if the docs refer to 'algorithm name'.
2. If algorithm name is not specified by the user, it can read an rcparam just like contour.corner_mask, which will initially be 'mpl2014'.
3. Probably should validate the name in mpl against the list of acceptable strings. This already happens in contourpy of course but we should protect against the possibility of dodgy installs combining old and new version of contourpy/mpl.
4. We'll allow name='mpl2005' (the original non-corner mask algorithm) and name='threaded' (experimental multithreaded version of 'serial'). There shouldn't be many users of either but the former can be used by people who have been using the legacycontour repo (for whatever reason?) and the latter will improve and become a reasonable option eventually.
5. Keep all of the existing tests as they are using 'mpl2014', and add one or two new ones to test the different algorithms. This is mostly to test that mpl and contourpy are talking to each other correctly, comprehensive testing off all algorithms occurs in contourpy.
6. Update docs, change note and API change for new kwarg. The docs will have to list the valid kwarg values but should avoid too much description and link to contourpy docs instead.
7. Doesn't need a new example, there is effectively no change for end users who are leaving the kwarg alone.

Once this is in 3.6 and we have confirmed everything is OK, a future PR will:

1. Switch the default to 'serial', via the rcparam.
2. Update the docs to explain the change.
3. Update the contour test baseline images to use 'serial' not 'mpl2014', and remove some of the tests as most of that responsibility is devolved to contourpy.

After that, there is the possibility of taking advantage of the improvements in 'serial', i.e.

1. Performance improvements by returning contour data in slightly different format from contourpy.
2. New functionality such as quad_as_tri option and different z_interp options.

I am keen on doing item 1. Item 2 might be driven by user requests for such.

Updates following yesterday's call:

1. Keyword argument will be called algorithm.
2. There is a new release of contourpy (v1.0.2) as requested by @QuLogic following the changes to the tests.

https://bugzilla.redhat.com/show_bug.cgi?id=2073692; no issues with tests on other architectures.

I'd squash it.

Commits squashed.

@QuLogic pinging you to review and merge if you are OK with this. I think this was all as discussed, but you may have packaging insights, if any. Thanks!

@ianthomas23 I squashed and merged your latest changes. Thanks for doing this - hopefully this is a smooth transition!