Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fixed bug in find_nearest_contour#22767

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

andrew-fennell
Copy link
Contributor

@andrew-fennellandrew-fennell commentedApr 1, 2022•
edited
Loading

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 (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

@andrew-fennellandrew-fennellforce-pushed thefind-nearest-contour-patch branch fromc51aeef to55d98a7CompareApril 1, 2022 19:14
@oscargusoscargus added this to thev3.5.2 milestoneApr 1, 2022
oscargus
oscargus previously approved these changesApr 4, 2022
@oscargus
Copy link
Member

Power cycling to trigger the tests.

@oscargusoscargus reopened thisApr 4, 2022
@andrew-fennell
Copy link
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

andrew-fennell commentedApr 8, 2022•
edited
Loading

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 theindices 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-fennellandrew-fennellforce-pushed thefind-nearest-contour-patch branch from55d98a7 to6eea6e6CompareApril 8, 2022 01:58
@andrew-fennell
Copy link
ContributorAuthor

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

#22762 highlights that currently, when the function is called oncontourf (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 changesApr 8, 2022
@QuLogic
Copy link
Member

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

andrew-fennell reacted with thumbs up emoji

@jklymak
Copy link
Member

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

@tacaswelltacaswell 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 reacted with thumbs up emoji
@andrew-fennell
Copy link
ContributorAuthor

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-fennellandrew-fennellforce-pushed thefind-nearest-contour-patch branch 2 times, most recently fromec55df9 tof8a80f7CompareApril 11, 2022 19:00
@tacaswelltacaswell dismissed stale reviews fromQuLogic andoscargusApril 11, 2022 22:11

Completely different approach now.

@tacaswelltacaswell merged commit6d42cf7 intomatplotlib:mainApr 12, 2022
@andrew-fennellandrew-fennell deleted the find-nearest-contour-patch branchApril 12, 2022 01:08
@QuLogic
Copy link
Member

@meeseeksdev backport to v3.5.x

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestApr 12, 2022
tacaswell added a commit that referenced this pull requestApr 12, 2022
…767-on-v3.5.xBackport PR#22767 on branch v3.5.x (Fixed bug in find_nearest_contour)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@QuLogicQuLogicQuLogic approved these changes

@oscargusoscargusAwaiting requested review from oscargus

Assignees
No one assigned
Projects
None yet
Milestone
v3.5.2
Development

Successfully merging this pull request may close these issues.

[Bug]: Issue with find_nearest_contour in contour.py
5 participants
@andrew-fennell@oscargus@tacaswell@QuLogic@jklymak
[8]ページ先頭
©2009-2025 Movatter.jp