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
Appearance settings

Separate alpha and rbg interpolation then recombine to fix issue11316 #12062

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 2 commits into from
Mar 1, 2019
Merged

Separate alpha and rbg interpolation then recombine to fix issue11316 #12062

merged 2 commits into from
Mar 1, 2019

Conversation

dabana
Copy link
Contributor

@dabana dabana commented Sep 7, 2018

PR Summary

Addresses issue #11316.

@tacaswell suggested two solutions:

  1. to change the output array starting as (1, 1, 1, 0)
  2. re-sample the RGB and A arrays separatly

I tested both solutions. Solution 1 did not work. I tested various output arrays but it does not have any impact on the result: grey boundaries are still visible. Finally, solution was the right one. I interpolate the alpha channel and the 3 rgb channels a 2 separate images. I then recombine into a 4-channel RGBA output.

This is a crude fix to the issue. Help wanted to improve elegance and/or efficiency.

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

@jklymak
Copy link
Member

jklymak commented Sep 7, 2018

Thanks @dabana Looks promising - I'm surprised this doesn't hit any tests. Since it doesn't, can you make some?

tacaswell
tacaswell requested changes Sep 7, 2018
View reviewed changes
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.

Overall 👍

Left some comments about making the computation more efficient (both memory and cpu)

lib/matplotlib/image.py Outdated Show resolved Hide resolved
lib/matplotlib/image.py Show resolved Hide resolved
@tacaswell tacaswell added this to the v3.1 milestone Sep 7, 2018
@tacaswell
Copy link
Member

Thanks @dabana ! Can you also add a test of this? The example in original issue would be a perfect test image.

jklymak
jklymak reviewed Sep 7, 2018
View reviewed changes
lib/matplotlib/tests/test_image.py Outdated Show resolved Hide resolved
@dabana
Copy link
Contributor Author

dabana commented Sep 14, 2018

@tacaswell I believe this is ready to merge.

@timhoffm
Copy link
Member

I don‘t have a complete overview of the topic, but can we save the alpha interpolation if there is no alpha involved?

tacaswell
tacaswell approved these changes Feb 26, 2019
View reviewed changes
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.

Who ever merges this should use squash merge if the duplicate images are not rebased out.

@tacaswell
Copy link
Member

@dabana Sorry this sat for so long. Could you possible resbase / squash this a bit so that we only have one copy of the images you added?

@dabana
Copy link
Contributor Author

dabana commented Feb 26, 2019

@dabana Sorry this sat for so long. Could you possible resbase / squash this a bit so that we only have one copy of the images you added?

Sorry @tacaswell I don't think I am getting what you mean. I just synced my fork with the base repo and I rebased this branch with the master. There were no conflicts and I do not see any duplicates of my images in the baseline_image directory.

@jklymak
Copy link
Member

jklymak commented Feb 26, 2019

@dabana the 8 commits have different versions of the image files in them. If we merge as is, each version of the images will be put in the repo separately, despite the fact the last version is the one we want.

@jklymak
Copy link
Member

jklymak commented Feb 26, 2019

For how to squash, the somewhat poorly named link here should help: Please do the backup step 😉! https://matplotlib.org/devel/gitwash/development_workflow.html#rewriting-commit-history

@tacaswell
Copy link
Member

@dabana you missed the force push step (the git command line helpfully provides you safe but wrong advise)

I think you want to do

git checkout fix-for-issue11316
git resest --hard afa6123  # THIS ONE IS A BIT DANGEROUS, but `git reflog` can save you
git push --force-with-lease   # THIS ONE IS THE DANGEROUS ONE, but more force pushes can help

@dabana dabana force-pushed the fix-for-issue11316 branch from 9544b38 to afa6123 Compare February 26, 2019 03:09
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 26, 2019
@tacaswell
Copy link
Member

the docs CI is failing because it runs on the branch head instead of simulating the merge and then running the tests so you don't have the fixes from master branch for these docs build issues.

@dabana
Copy link
Contributor Author

dabana commented Feb 26, 2019

the docs CI is failing because it runs on the branch head instead of simulating the merge and then running the tests so you don't have the fixes from master branch for these docs build issues.

Is there anything I can do about it to help the merge?

@tacaswell
Copy link
Member

If you re-base the branch on to current master (and then force-push again) the docs will pass, however I don't think we are going to hold the PR over that.

We are just waiting for a second core-dev to review and merge this.

dstansby
dstansby approved these changes Mar 1, 2019
View reviewed changes
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

👍 thanks a lot!

@dstansby dstansby merged commit 7effa1e into matplotlib:master Mar 1, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 1, 2019
@dstansby
Backport PR matplotlib#12062: Separate alpha and rbg interpolation th…
da1245f 
…en recombine to fix issue11316
@meeseeksmachine meeseeksmachine mentioned this pull request Mar 1, 2019
Merged
dstansby added a commit that referenced this pull request Mar 1, 2019
@dstansby
Merge pull request #13559 from meeseeksmachine/auto-backport-of-pr-12…
4fe1b00 
…062-on-v3.1.x

Backport PR #12062 on branch v3.1.x (Separate alpha and rbg interpolation then recombine to fix issue11316)
@tacaswell tacaswell mentioned this pull request May 17, 2019
Closed
@anntzer anntzer mentioned this pull request Mar 17, 2025
Open
@anntzer anntzer mentioned this pull request Mar 18, 2025
Merged
5 tasks
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

@tacaswell tacaswell tacaswell approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.
6 participants
@dabana @jklymak @tacaswell @timhoffm @dstansby @QuLogic
Morty Proxy This is a proxified and sanitized view of the page, visit original site.