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

Fixed bug in find_nearest_contour #22767

Merged

Conversation

andrew-fennell
Copy link
Contributor

@andrew-fennell andrew-fennell commented Apr 1, 2022

PR Summary

The find_nearest_contour function was defined incorrectly. I fixed the bug and added a simple test, based on the original issues test.

Thank you @jhelmboldt for finding this issue and offering a solution to it.

Closes #22762

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

@oscargus oscargus added this to the v3.5.2 milestone Apr 1, 2022
oscargus
oscargus previously approved these changes Apr 4, 2022
@oscargus
Copy link
Contributor

oscargus commented Apr 4, 2022

Power cycling to trigger the tests.

@oscargus oscargus closed this Apr 4, 2022
@oscargus oscargus reopened this Apr 4, 2022
@andrew-fennell
Copy link
Contributor Author

I think this PR is ready for further review.

Let me know if any changes need to be made to the testing.

@@ -1371,7 +1371,7 @@ def find_nearest_contour(self, x, y, indices=None, pixel=True):
# Nonetheless, improvements could probably be made.

if indices is None:
indices = range(len(self.levels))
indices = range(len(self.layers))
Copy link
Member

Choose a reason for hiding this comment

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

I have a slight preference to change this to

Suggested change
indices = range(len(self.layers))
indices = range(len(self.collections))

as that is the data structure that we need to match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this change makes sense. I outlined some issues below that I'd like you to look at.

@tacaswell
Copy link
Member

I have not chased this through enough to answer this my self, but is this method implemented in a way that makes sense for filled contours? With line contours you know that each path is at the same level and lines never intersect. With a filled contour the whole patch is "at the same level" with discontinuities at the boundaries (that is there are two patches that share and edge that are different values).

In the case of filled shouldn't we be saying "which collection does this fall in?" instead?

@andrew-fennell
Copy link
Contributor Author

andrew-fennell commented Apr 8, 2022

is this method implemented in a way that makes sense for filled contours?

Upon further testing, I'm not sure that the current function is a completely suitable for filled contours. Regardless of the indices being defined by then length of layers/collections.

I marked the input point and the output point (of the nearest contour) on each of the following filled contour examples with a red dot.

Is this expected behavior of this function? The output for line contours makes logical sense, but the 3rd example shows (from my understanding) that this method isn't working on filled contours.

Test 1 - Good

Filled contour, where point given is not on a layer:
image

Call: contourf.find_nearest_contour(1.5, 7, pixel=False)
Output: (2, 1, 3, 2.3, 7.4, 0.7999999999999999)

Test 2 - Good

Filled contour, where point given is on a layer:
image

Call: contourf.find_nearest_contour(3.6, 8.8, pixel=False)
Output: (2, 1, 3, 2.3, 7.4, 0.7999999999999999)

Test 3 - Bad

This all fails on a plot like this though:
image

Call: contourf.find_nearest_contour(1,1,pixel=False)
Output: (0, 0, 57, 1.3436129813210376, 0.13677814070781982, 0.8632218592921803)

@andrew-fennell
Copy link
Contributor Author

If this function should only support contour (line contours), it should probably be documented that way.

#22762 highlights that currently, when the function is called on contourf (filled contours) it crashes. That would be fixed with this PR, but I'm not convinced the outputs are correct in every situation.

QuLogic
QuLogic previously approved these changes Apr 8, 2022
@QuLogic
Copy link
Member

QuLogic commented Apr 8, 2022

Sorry, didn't see @tacaswell's additional questions; I won't merge until he's satisfied with the answers.

@jklymak
Copy link
Member

jklymak commented Apr 8, 2022

I am not sure what use there is in choosing the nearest "contour" on a filled contour. My tendency would be to raise on contourf collections and if users need to know the "nearest contour" they can make an unfilled contour to go with the filled one (perhaps hiding it if they don't want itvnisually there).

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

The consensus of the discussion is that the correct fix here is to :

  • document that this only works on unfilled contours
  • make it raise a better error on filled contours

@andrew-fennell
Copy link
Contributor Author

I agree with the consensus! Thanks for all of the input.

I will make these changes soon. I need to get through a few other things first.

@andrew-fennell andrew-fennell force-pushed the find-nearest-contour-patch branch 2 times, most recently from ec55df9 to f8a80f7 Compare April 11, 2022 19:00
@tacaswell tacaswell dismissed stale reviews from QuLogic and oscargus April 11, 2022 22:11

Completely different approach now.

@tacaswell tacaswell merged commit 6d42cf7 into matplotlib:main Apr 12, 2022
@andrew-fennell andrew-fennell deleted the find-nearest-contour-patch branch April 12, 2022 01:08
@QuLogic
Copy link
Member

QuLogic commented Apr 12, 2022

@meeseeksdev backport to v3.5.x

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Apr 12, 2022
tacaswell added a commit that referenced this pull request Apr 12, 2022
…767-on-v3.5.x

Backport PR #22767 on branch v3.5.x (Fixed bug in find_nearest_contour)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Issue with find_nearest_contour in contour.py
5 participants