Skip to content

[Bug]: find_nearest_contour deprecated with no replacement? #27070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jklymak opened this issue Oct 12, 2023 · 9 comments · Fixed by #27088
Closed

[Bug]: find_nearest_contour deprecated with no replacement? #27070

jklymak opened this issue Oct 12, 2023 · 9 comments · Fixed by #27088

Comments

@jklymak
Copy link
Member

jklymak commented Oct 12, 2023

Bug summary

In #25247 we deprecated find_nearest_contour with no replacement or mention in the release notes. Why was this deprecated, and what is the replacement? ping @anntzer @ianthomas23

https://github.com/matplotlib/matplotlib/blob/c06a97e84b8424a87d0e294c5eaf406a1f8d0a62/lib/matplotlib/contour.py#L1391C5-L1392C68

@anntzer
Copy link
Contributor

anntzer commented Oct 12, 2023

It was deprecated because the old return value (Collection (=level), index of Path in Collection (=index of connected component at that level), index of segment in Path, x, y, distance) doesn't really make much sense for the new internal representation of ContourSets (there's a single Collection, and a single path per level).
In particular there isn't really a good replacement for the first tuple item (we could return the containing collection but now that's the entire ContourSet, or dynamically generate a new Collection that contains just the paths for that level, but that seems rather contrived); the second tuple item would also be a bit weird to replace now (we could still say what connected component is closest, or just return 0 because it's always the path at index zero that's relevant (as that's the only path anyways for that level?)
The internal replacement is _find_nearest_contour, which more or less does the same thing but returns values that make sense for the new internal representation: the index of the level, the index of the vertex in that level's (multi-component) path, and the position of the nearest point as a pair (as I find that better than returning x, y separately). I stopped returning the distance as that's not something actually needed, and it can easily be computed anyways from the coordinates of the original point and of the closest projection.
If there is really demand for it, I wouldn't mind making the new _find_nearest_contour public (perhaps with a new name).

@jklymak
Copy link
Member Author

jklymak commented Oct 12, 2023

If there is really demand for it, I wouldn't mind making the new _find_nearest_contour public (perhaps with a new name).

https://stackoverflow.com/questions/77281818/find-nearest-contour-is-deprecated-now-what

I don't personally need "nearest contour" but you can imagine it would be very useful to people. I imagine the poster's use case would be met by some public equivalent...

@anntzer
Copy link
Contributor

anntzer commented Oct 12, 2023

Let's just make _find_nearest_contour public under a different name.

@matteobachetti
Copy link

matteobachetti commented Oct 13, 2023

@jklymak thanks for sharing my question on Stack Overflow. I have code that depends on this functionality, and deprecations make me nervous :)
@anntzer If you think that the private method will be stable for the foreseeable future, I'll be happy to use that. Still, I think this is useful functionality that might deserve a public method.

@matteobachetti
Copy link

matteobachetti commented Oct 13, 2023

@anntzer actually, _find_nearest_contour won't work for me.

To give you context, I'm using Matplotlib contours to explore a 2d map. I'm usingfind_nearest_contour to get the range of x and y of the contour that passes close to a point x0, y0, as follows:

    cs = fig.gca().contour(x, y, image, [level])

    cont, seg, idx, xm, ym, d2 = cs.find_nearest_contour(x0, y0, pixel=False)

    min_x = cs.allsegs[cont][seg][:, 0].min()
    max_x = cs.allsegs[cont][seg][:, 0].max()
    min_y = cs.allsegs[cont][seg][:, 1].min()
    max_y = cs.allsegs[cont][seg][:, 1].max()

The point is that there are multiple contours created at level, but I only want the one close to x0, y0. find_nearest_contour was perfect, and _find_nearest_contour does not share the index of the nearest path (what I call seg).

@matteobachetti
Copy link

I now realize that also allsegs has changed, and it has a different dimension with respect to before. This not only throws a deprecation, but it breaks the existing functionality :(

@rcomer
Copy link
Member

rcomer commented Oct 13, 2023

@matteobachetti the previous allsegs behaviour will be re-instated at v3.8.1 - #26908.

@anntzer
Copy link
Contributor

anntzer commented Oct 13, 2023

Ah, looking at this again I missed the fact that the first tuple item returned by find_nearest_contour is not a Collection (despite what the docstring says), but the index of the collection (effectively the level index).
I guess in that case it is indeed possible to rewrite find_nearest_contour to use the new API but still return values such that cs.allsegs[cont][seg] returns whatever you need, without accessing cs.collections (which should definitely stay deprecated, as removing the "multiple-collections" representation is more or less the whole point of the API break).

@anntzer
Copy link
Contributor

anntzer commented Oct 14, 2023

From a quick look, the old find_nearest_contour can be implemented in terms of the new one as follows (modulo off-by-one errors):

from contextlib import ExitStack

...

def find_nearest_contour(...):
    with ExitStack() as stack:
        if not pixel:
            stack.enter_context(self._cm_set(
                transform=mtransforms.IdentityTransform()))
        i_level, i_vtx, (xmin, ymin) = self._find_nearest_contour((x, y), indices)
    cc_cumlens = np.cumsum(
        [*map(len, self._paths[i_level]._iter_connected_components())])
    segment = cc_cumlens.searchsorted(i_vtx, "right") - 1
    index = i_vtx - cc_cumlens[segment]
    return (i_level, segment, index, xmin, ymin, (xmin-x)**2 + (ymin-y)**2)

@QuLogic QuLogic added this to the v3.8.1 milestone Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants