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

Fixing Bug with RGB photos in imshow using alpha array-type #26144

Closed
wants to merge 5 commits into from

Conversation

NikosNikolaidis02
Copy link

@NikosNikolaidis02 NikosNikolaidis02 commented Jun 18, 2023

PR summary

"
This pull request tries to ressolve issue #26092
I've tried to added the proposal solution of @tacaswell , but I would like your feedback what else I should try to include to ressolve this issue.
"

PR checklist

@rcomer
Copy link
Member

rcomer commented Jun 18, 2023

Thank you for your contribution @NikosNikolaidis02! Please could you have a look through the checklist in the PR summary above and also the pull request guidelines in the developers' guide. In particular

  • This will need new tests to prove that the code change has the desired effect.
  • The existing tests need to pass: currently four are failing.
  • We appreciate the PR title to describe what the change is about.

I'm going to mark this as "draft" for now. Please mark it as "ready for review" when you are ready. In the meantime, if you need help, feel free to ask questions here. Or you may prefer to ask them in our incubator gitter room.

@rcomer rcomer marked this pull request as draft June 18, 2023 12:21
@NikosNikolaidis02 NikosNikolaidis02 changed the title Resolving issue #26092 Fixing Bug with RGB photos in imshow using alpha array-type Jun 19, 2023
@NikosNikolaidis02
Copy link
Author

Thank you for your contribution @NikosNikolaidis02! Please could you have a look through the checklist in the PR summary above and also the pull request guidelines in the developers' guide. In particular

  • This will need new tests to prove that the code change has the desired effect.
  • The existing tests need to pass: currently four are failing.
  • We appreciate the PR title to describe what the change is about.

I'm going to mark this as "draft" for now. Please mark it as "ready for review" when you are ready. In the meantime, if you need help, feel free to ask questions here. Or you may prefer to ask them in our incubator gitter room.

Hello @rcomer,

Thank you for your fast response!

I will try to fix these issues in these days, so I would really like your guidelines through the process!

@rcomer
Copy link
Member

rcomer commented Jun 21, 2023

Tests are failing because of a new release of one of our dependencies (#26152). We pinned that at #26153, so hopefully if we close and re-open this PR it will pick up that change.

@rcomer rcomer closed this Jun 21, 2023
@rcomer rcomer reopened this Jun 21, 2023
@NikosNikolaidis02
Copy link
Author

@rcomer should I ignore the failure on macos?

@QuLogic
Copy link
Member

QuLogic commented Jun 21, 2023

That is also a known issue that can be ignored for now.

@NikosNikolaidis02
Copy link
Author

So when my pull request will be open to be reviewed?

@rcomer
Copy link
Member

rcomer commented Jun 22, 2023

At the moment it looks like you only applied the patch from #26092 (comment), but that comment also says that that patch is not the whole solution. To proceed, we would need a complete solution, including new tests to confirm the behaviour is now as we want it.

I do not want to discourage you from contributing in Matplotlib but I will note that the issue is marked "medium difficulty". So if you are inexperienced with python or testing, it may be better to choose a different issue to work on initially.

@NikosNikolaidis02
Copy link
Author

Ok, thank you! I will try to continue with this issue, but I would appreciate your help on testing on which I am unexperienced.

@rcomer
Copy link
Member

rcomer commented Jun 24, 2023

We have guidance about writing tests here:
https://matplotlib.org/stable/devel/testing.html

But I also see from #26092 (comment) that the current change

does not address the "what about RGBA" issue and I think will double apply the alpha in scalar cases.

So there is probably also still more to do in the main code.

@rcomer
Copy link
Member

rcomer commented Jul 16, 2023

Hi @NikosNikolaidis02 I’m guessing you decided not to pursue this PR since you opened one for a different issue. So I’m going to close it so it’s more obvious to others that they can work on it.

If you are still interested in working on this, please do comment and we can re-open.

@rcomer rcomer closed this Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

[Bug]: alpha array-type not working with RGB image in imshow()
3 participants