Skip to content

Adds support for rgba and rgb images to pcolorfast #8690

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
Mar 18, 2019
Merged

Adds support for rgba and rgb images to pcolorfast #8690

merged 1 commit into from
Mar 18, 2019

Conversation

paalge
Copy link
Contributor

@paalge paalge commented May 31, 2017 •
edited
Loading

PR Summary

This pull request fixes issue #1317, by making nr, and nc equal to the first and second element of C.shape.
Through this, pcolorfast now works on RGBA arrays, i.e. NxMx3 or NxMx4.

PR Checklist

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

@efiring
Copy link
Member

efiring commented Jun 5, 2017

Please look again at the discussion in #1317. This simple 1-line change, by itself, is not enough.

@paalge
Copy link
Contributor Author

paalge commented Jun 6, 2017

I have read the discussion, and I agree the one line is not enough, though it fixes half of the problem, instead of none of it as it has been for the last years, while generating an exception (as it does now) for some complicated grids. This PR fixes problems I had with the code, and would for others as well.

@dopplershift
Copy link
Contributor

Reading the discussion, it seems like there wasn't anything against making this particular change, though.

@efiring
Copy link
Member

efiring commented Jun 6, 2017

Correct. It just needs a complete PR, with more than the one-line change.

  1. Additions to the docstring explaining the new behavior, and when it is available.
  2. Validation with an informative error message, to trap the case where a 3-D array makes its way into the quadmesh code path.
  3. A note in doc/api/api_changes.

@dopplershift
Copy link
Contributor

Thank you @efiring for being explicit in what changes you were requesting... 😁

@paalge
Copy link
Contributor Author

paalge commented Jun 8, 2017

Thank you @efiring for specifying what you wanted.
I've pushed some changes now that should address these.

@tacaswell tacaswell added this to the 2.2 (next next feature release) milestone Sep 24, 2017
@paalge
Copy link
Contributor Author

paalge commented Mar 7, 2018

I see this was not part of the v/2.2 release. What is needed to get this into matplotlib?

@anntzer
Copy link
Contributor

anntzer commented Jan 30, 2019

Needs a rebase, but I think something like this can go in? Possibly with some tests as well...

efiring
efiring requested changes Jan 30, 2019
View reviewed changes
Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

In addition to the minor doc edits, rebase, and updating the api_changes filename, I recommend adding a test.

@paalge
Copy link
Contributor Author

paalge commented Feb 11, 2019

I've added a test, minor doc change and rebased.
I hope it is okay now?

anntzer
anntzer reviewed Feb 11, 2019
View reviewed changes
anntzer
anntzer reviewed Feb 11, 2019
View reviewed changes
anntzer
anntzer reviewed Feb 11, 2019
View reviewed changes
jklymak
jklymak reviewed Feb 11, 2019
View reviewed changes
anntzer
anntzer reviewed Feb 26, 2019
View reviewed changes
anntzer
anntzer reviewed Feb 26, 2019
View reviewed changes
anntzer
anntzer reviewed Feb 26, 2019
View reviewed changes
anntzer
anntzer reviewed Feb 26, 2019
View reviewed changes
@anntzer
Copy link
Contributor

anntzer commented Feb 26, 2019

Also, consider perhaps squashing your commits.

anntzer
anntzer reviewed Feb 26, 2019
View reviewed changes
@anntzer anntzer modified the milestones: needs sorting, v3.1.0 Feb 26, 2019
@anntzer
Copy link
Contributor

anntzer commented Feb 26, 2019

I would like something like this to make it to 3.1, especially considering that it's quite close to being mergeable. Feel free to remilestone if you disagree.

@paalge
Copy link
Contributor Author

paalge commented Feb 27, 2019

I've addressed the comments from @anntzer.
I'll squash the commits if there are no other comments.

anntzer
anntzer reviewed Feb 27, 2019
View reviewed changes
@anntzer
Copy link
Contributor

anntzer commented Feb 27, 2019

Only one minor comment left, you can fix and squash.

@jklymak jklymak modified the milestones: v3.1.0, v3.2.0 Mar 3, 2019
@jklymak
Copy link
Member

jklymak commented Mar 3, 2019

Feel free to re-milestone if this gets done sooner, but I don't think its urgent...

@paalge
Copy link
Contributor Author

paalge commented Mar 5, 2019

I've changed the last comment, but I having problems squashing the pull request due to the rebases (4300 commands that git needs to do). This could be due to my inexperience in squashing. If it can't be done in when you merge the code into master, my solution would be to close this and move the code to a new pull request based on the latest master

@anntzer
Copy link
Contributor

anntzer commented Mar 5, 2019

Do you mind if I force-push to your branch to fix that?

@paalge
Copy link
Contributor Author

paalge commented Mar 5, 2019

@anntzer Go ahead, you're more than welcome.

@paalge @anntzer
Pull request fixing the long standing issue matplotlib#1317
a9801b6 
Added doc string and raise of error for quadmesh

Added test for RGB image in pcolorfast

Added doc changes per efirings comments

Doc change and use of pytest.raises

Changed from TypeError to ValueError

Removed whitespace

Doc changes in response to comments

Doc modified
@anntzer
Copy link
Contributor

anntzer commented Mar 5, 2019

Rebased.

@anntzer anntzer modified the milestones: v3.2.0, v3.1.0 Mar 5, 2019
@anntzer
Copy link
Contributor

anntzer commented Mar 5, 2019

Good to go for 3.1.0 IMO.

@efiring efiring merged commit 94b0bca into matplotlib:master Mar 18, 2019
@lumberbot-app
Copy link

lumberbot-app bot commented Mar 18, 2019

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout v3.1.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 94b0bca18446c9529a0373400e731ca9b5a0b0f2
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #8690: Adds support for rgba and rgb images to pcolorfast'
  1. Push to a named branch :
git push YOURFORK v3.1.x:auto-backport-of-pr-8690-on-v3.1.x
  1. Create a PR against branch v3.1.x, I would have named this PR:

"Backport PR #8690 on branch v3.1.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Mar 19, 2019
@efiring @tacaswell
Merge pull request matplotlib#8690 from paalge/pcolorfast
46a5bea 
Adds support for rgba and rgb images to pcolorfast

Conflicts:
	lib/matplotlib/axes/_axes.py
          - C.shape -> np.shape(C) bug fix was not backported
@tacaswell tacaswell mentioned this pull request Mar 19, 2019
Merged
6 tasks
QuLogic added a commit that referenced this pull request Mar 19, 2019
@QuLogic
Merge pull request #13709 from tacaswell/auto-backport-of-pr-8690-on-…
afc4e61 
…v3.1.x

Backport PR #8690 on branch v3.1.x
@paalge paalge deleted the pcolorfast branch March 22, 2019 14:53
@anntzer anntzer mentioned this pull request Apr 18, 2019
Merged
6 tasks
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request May 18, 2019
@tacaswell
Merge remote-tracking branch 'origin/v3.1.x'
2ccb939 
Resolved all conflicts in favor of the master branch except for the
changes to doc/api/prev_api_changes/api_changes_3.1.0.rst

pcolorfast was updating to take RGB(A) in some cases in PR matplotlib#8690 /
94b0bca that was backported to v3.1.x
via matplotlib#13709 / afc4e61 .  However, the
functionality was further extended in matplotlib#13986 /
c05537d which was not backported.
When merging v3.1.x into master special care had to be taken to not
pull the removed code back into master.
@efiring efiring mentioned this pull request May 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak left review comments

@efiring efiring efiring approved these changes

@tacaswell tacaswell tacaswell approved these changes

@anntzer anntzer anntzer approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@paalge @efiring @dopplershift @anntzer @jklymak @tacaswell @QuLogic