8000 pcolorfast simplifications. by anntzer · Pull Request #13327 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

pcolorfast simplifications. #13327

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 28, 2019
Merged

pcolorfast simplifications. #13327

merged 1 commit into from
Feb 28, 2019

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented Jan 30, 2019

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

anntzer
anntzer commented Jan 30, 2019
View reviewed changes
lib/matplotlib/axes/_axes.py
@@ -6213,7 +6213,7 @@ def pcolorfast(self, *args, alpha=None, norm=None, cmap=None, vmin=None,
"'norm' must be an instance of 'mcolors.Normalize'")

C = args[-1]
nr, nc = C.shape
nr, nc = np.shape(C)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(supports C being a nested list instead of an array)

jklymak
jklymak previously requested changes Jan 30, 2019
View reviewed changes
Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Blocking on the discussion of the future of pcolorfast. See #13328. Of course, this can be cleared if we think pcolorfast should get more work, but as discussed, I think we should consider deprecating it.

@timhoffm
Copy link
Member

This wouldn't do any harm even if we decide to deprecate pcolorfast.

@jklymak jklymak dismissed their stale review February 1, 2019 15:02

I'll unblock - but I don't think we should be improving this method.

@ImportanceOfBeingErnest
Copy link
Member

I would be supportive of keeping pcolorfast, even if it is in general of course really bad style to have the return type of the function depend on some (possibly complex) interplay of input parameters. That being said, pcolorfast is almost completely untested. So if a test could be added to check the return type and limits depending on all combinations of input options that would really help.

@anntzer anntzer mentioned this pull request Feb 14, 2019
Merged
6 tasks
@anntzer
Copy link
Contributor Author
anntzer commented Feb 14, 2019

See #13434 for some more testing of pcolorfast.

@jklymak
Copy link
Member
jklymak commented Feb 14, 2019

My issue w/ pcolorfast is that it is redundant with pcolormesh and imshow. Again, asside from the API being different, it doesn't do anything that those two methods don't do.

@anntzer
Copy link
Contributor Author
anntzer commented Feb 14, 2019 •
edited
Loading

Let's keep that discussion in #13328, but in that thread I made the point that I disagree with the assessment that the two methods "provide" similar functionalities. Using imshow() to draw an array so that a certain data entry ends up centered at a certain x, y coordinate is, as far as I can tell, extremely awkward (in particular because of the complex interplay between the extent and the origin kwargs). It is true that anything pcolorfast does can be translated into an imshow/pcolorfast/pcolormesh call, but that's like saying that we don't need pie() because we could get the same thing by manually drawing slices on a polar plot with bar().

@ImportanceOfBeingErnest
Copy link
Member

What this PR does is to simplify the internal code of an existing, and for some users viable function. It doesn't add anything, and it doesn't remove anything. So I wouldn't spend too much time with it. Only thing is that such "simplifications" are always a good opportunity to add lacking tests, so if possible, I would like to see #13434 be merged first.

@timhoffm
Copy link
Member

@ImportanceOfBeingErnest #13434 is merged.

@anntzer
Copy link
Contributor Author
anntzer commented Feb 26, 2019

@ImportanceOfBeingErnest do you want to get this in? :)

@ImportanceOfBeingErnest ImportanceOfBeingErnest merged commit 9a8df49 into matplotlib:master Feb 28, 2019
@QuLogic QuLogic added this to the v3.2.0 milestone Feb 28, 2019
@anntzer anntzer deleted the pcolorfast branch February 28, 2019 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm approved these changes

@ImportanceOfBeingErnest ImportanceOfBeingErnest ImportanceOfBeingErnest approved these changes

@jklymak jklymak jklymak left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@anntzer @timhoffm @ImportanceOfBeingErnest @jklymak @QuLogic
0