Skip to content
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

Update find_nearest_contour and revert contour deprecations #27088

Merged
merged 2 commits into from Oct 26, 2023

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Oct 14, 2023

PR summary

  • Following [Bug]: find_nearest_contour deprecated with no replacement? #27070 (comment), this PR updates the ContourSet.find_nearest_contour method so it no longer uses the deprecated collections attribute.

    Note that the docstring previously stated in error that the first element of the returned tuple is a Collection. It was actually an integer that indexed the collections attribute. Since <3.8 behaviour had one collection per level, and in 3.8 we have one path per level I have updated this to say the index of the path.

  • contour.allsegs, contour.allkinds, and contour.find_nearest_contour are no longer marked for deprecation.

Closes #27070 and closes #26913

PR checklist

segment : int
The index of the `.Path` in *contour* that is closest to
The index within that closest path of the segment that is closest to
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using "segment" here to mean one of the pieces you get when you split the path by its MOVETOs. I'm not sure I have the language right though. What exactly defines a segment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connected component, I think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word 'segment' is a bit confusing, because Paths have a method iter_segments, which splits by code, i.e., a MOVETO returns one point, a CURVE3 returns two points, etc. This does not appear to be the same as the 'segment' used here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I change “segment” to “connected component” here? Or something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it could be a 'subpath'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"subpath" is what I have called the variable when I've looped through these, so that is intuitive to me 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I have changed it to "subpath" and also added some explanation of what paths and subpaths correspond to in the plot.

@rcomer
Copy link
Member Author

rcomer commented Oct 14, 2023

Updated based on #27070 (comment). There is a possibility for all the return values to be None, but I think this can only happen if the paths are all empty, which seems artificial. There is also no test for that case.

@anntzer
Copy link
Contributor

anntzer commented Oct 14, 2023

I have a slight preference for using the _cm_set approach for handling the pixel=False (if that indeed works, untested) so that the backcompat code doesn't start interfering with the "main intended usage" code.

@rcomer
Copy link
Member Author

rcomer commented Oct 15, 2023

It does work. It just seemed odd to me to essentially hack the method's behaviour from outside when directly changing the behaviour was very simple. I will defer to you on that though.

@tacaswell tacaswell added this to the v3.8.1 milestone Oct 18, 2023
@rcomer rcomer changed the title Update find_nearest_contour to not use collections attribute Update find_nearest_contour and revert contour deprecations Oct 19, 2023
rcomer and others added 2 commits October 26, 2023 19:10
Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
@ksunden ksunden merged commit 3ecb148 into matplotlib:main Oct 26, 2023
40 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 26, 2023
QuLogic added a commit that referenced this pull request Oct 27, 2023
…088-on-v3.8.x

Backport PR #27088 on branch v3.8.x (Update `find_nearest_contour` and revert contour deprecations)
@rcomer rcomer deleted the nearest-contour branch October 28, 2023 17:26
@ksunden ksunden mentioned this pull request Nov 2, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants