Skip to content

ENH: Add option to define a color as color=(some_color, some_alpha) #24691

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

Merged
merged 1 commit into from
Mar 19, 2023

Conversation

j1642
Copy link
Contributor

@j1642 j1642 commented Dec 10, 2022

PR Summary

In the Patch class, add the option of setting separate alpha values for facecolor and edgecolor using a tuple, alpha=(facecolor_alpha, edgecolor_alpha). Closes #24681.

Add (matplotlib supported color, alpha) as a valid matplotlib color specification

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

I think this is not the right API to aim for, because it doesn't generalize at all, e.g. for markerfacecoloralt or for gapcolor). Perhaps supporting (colorstring, alpha) pairs could be a solution but in any case this needs more discussion.

@story645
Copy link
Member

story645 commented Dec 11, 2022

Ack, sorry I didn't qualify my issue w/ a "please don't try to implement this yet!" b/c I threw my suggestion out there to seed discussion. (ETA: but also thanks for liking the idea enough to try and implement it!)

Perhaps supporting (colorstring, alpha) pairs could be a solution

I was thinking of that, but to some extent that is the RGBA solution - maybe alpha can be obnoxiously explicit and also take a dict alpha={'facecolor': , edgecolor:' ', markerfacealt:} ? Granted, I think at that point then maybe it is the priority problem again - if the color is RGBA does it take precedence or alpha? Which granted is also currently a question if I pass in an RGBA and an alpha anyway

@timhoffm
Copy link
Member

I have also been considering the dict-approach briefly. However, I don't think it's a good design. From the user point of view it's quite cumbersome to write facecolor='red', alpha={'facecolor': 0.5}. On the implementation side, we'd need special routing logic to apply the correct alpha key to the correct keyword. That may be somewhat automizable, but we have places where we can have "color or list of colors" and you'd need to fiddle alpha in that logic as well.

Conceptually, alpha is very closely related to color. Basically, you'll want to be able to make every color transparent. I think we should aim for a single entity that holds information on both. Some color specifications like RGBA already have that.

The formal way would be an explicit class Color('aquamarine', alpha=0.5). I think there were ideas at some point to introduce this internally as a way to standardize color handling.
The downside is that it's a bit more verbose when writing code and the user has to import that class. I could warm to the idea of having a tuple as a shortcut on the API side. color=('aquamarine', 0.5) is not too bad.

@anntzer
Copy link
Contributor

anntzer commented Dec 11, 2022

Granted, I think at that point then maybe it is the priority problem again - if the color is RGBA does it take precedence or alpha? Which granted is also currently a question if I pass in an RGBA and an alpha anyway

Currently, an explicit alpha overrides the alpha of a rgba tuple. (I think this is essentially a consequence of the semantics of to_rgba.) I think this is not particularly better or worse than the inverse semantics (having the alpha of rgba tuples win against explicit alphas), so we should just keep the current semantics in all cases.

@story645
Copy link
Member

story645 commented Dec 11, 2022

Color('aquamarine', alpha=0.5)

So I never checked the signature of to_rgba(c, alpha) 🤦‍♀️ so maybe better documenting/adding an example would help.

On the one hand I'm very pro an explicit 'Color' type, and it would essentially be doing to_rgba under the hood, but on the other not sure if we wanna go down that route yet. (ETA @ksunden what are your thoughts?)

Maybe '(color, alpha)' is the most straightforward approach since it's basically a thin wrapper to calling to_rgba. Eta: also if me, Anthony, and Tim all sorta converged there, it'll be intuitive once it's advertised?

@timhoffm
Copy link
Member

I think the way forward here is to declare ("red", 0.5) as an official way to define alpha for colors. We need to define that a bit more precise though: Should we allow this for the cases tuple and hex-string as well, which already have built-in alpha support? I'm inclined to not do this, which saves us extra checks and disambiguation.

Concrete steps:

I assume everything else will fall in place then.

@story645
Copy link
Member

story645 commented Dec 12, 2022

Should we allow this for the cases tuple and hex-string as well, which already have built-in alpha support? I'm inclined to not do this

My use case was HTML colors...I didn't know I could add alpha to them by concatenating an alpha value into hex until right now and I think "(anything considered a valid matplotlib color, alpha)" is more straighforward for the user than trying to special case a subset of the supported colors. Also, that is what's already supported in to_rgba, so it's a straight pass through. What I mean is this already works:

mpatches.Ellipse(ec, width=ew, height=eh, edgecolor=html_color, facecolor=mcolors.to_rgba(html_color, .10))

ETA: While I suggested an API change, maybe an alternative way to address this is another gallery example in the color section that shows how to use to_rgba to set different alpha values for different parts of a patch and line and marker and the like? Granted, I think we also should also add this sort of gallery example if (color name, alpha) becomes a supported spec too.

@timhoffm
Copy link
Member

You can also only document to_rgba, which is less work.

@story645
Copy link
Member

it is documented: https://matplotlib.org/devdocs/api/_as_gen/matplotlib.colors.to_rgba.html but all the examples could just as easily be written using to_rgb and so it's not obvious.

@anntzer anntzer mentioned this pull request Dec 12, 2022
6 tasks
@j1642 j1642 marked this pull request as draft January 30, 2023 01:55
@j1642
Copy link
Contributor Author

j1642 commented Jan 30, 2023

Here is an implemetation for ('color', alpha) if we want to go that route.

Alternatively, I like the idea of adding an alpha example to the color gallery. I noticed the colors tutorial has a clear alpha example, although it doesn't use to_rgba().

@j1642 j1642 marked this pull request as ready for review February 4, 2023 02:26
@jklymak jklymak requested a review from anntzer February 6, 2023 17:18
@story645
Copy link
Member

story645 commented Feb 6, 2023

@j1642 sorry for dropping the ball on this, I think it's still an open question on whether we want to add this additional color specification (I'm 👍) but if you're interested please open a separate PR to add an example to the docs.

ETA 2: if there's consensus, then also please add this spec to the table in https://matplotlib.org/devdocs/tutorials/colors/colors.html#sphx-glr-tutorials-colors-colors-py

@timhoffm
Copy link
Member

timhoffm commented Feb 7, 2023

I'm also inclined to add this specification.

While this is strictly redundant (color=(some_color, .5) is identical to color=mcolors.to_rgba(som_color, .5)), I think this is a usability topic. Not having to import and type mcolors.to_rgba is a sizable simplification. I also think the tuple notation is still readable enough despite being quite terse.

As opposed to my previous comment

Should we allow this for the cases tuple and hex-string as well, which already have built-in alpha support? I'm inclined to not do this, which saves us extra checks and disambiguation.

I'm quite firm now that we should allow this and not do any artificial limitations. First, the strict equivalence color=(some_color, .5) is identical to color=mcolors.to_rgba(som_color, .5) is preferable because of the simple logic. Second, there's a valid use case that one wants to add transparency to a color handed down from somewehere else. Making this work only for a subset of types is not reasonable.

@j1642 j1642 changed the title ENH: Separate facecolor alpha and edgecolor alpha for patches ENH: Add option to define a color as color=(some_color, some_alpha) Feb 7, 2023
@jklymak jklymak requested review from anntzer and timhoffm February 11, 2023 23:28
@jklymak jklymak added MEP: MEP needed and removed status: needs comment/discussion needs consensus on next step labels Feb 11, 2023
@story645
Copy link
Member

Sorry haven't merged cause haven't had the time to investigate if the ci failure is related.

@ksunden
Copy link
Member

ksunden commented Mar 17, 2023

Appveyor failure is unrelated, unfortunately was caught by #25209 so a conflict in the tutorial has arisen

@story645 story645 added this to the v3.8.0 milestone Mar 19, 2023
@story645 story645 merged commit 17fd9de into matplotlib:main Mar 19, 2023
@story645
Copy link
Member

Thanks for all your patience w/ this(us) @j1642!

@j1642
Copy link
Contributor Author

j1642 commented Mar 19, 2023

No problem. Thanks everyone!

@j1642 j1642 deleted the patch_alpha branch March 19, 2023 19:50
ksunden added a commit to ksunden/matplotlib that referenced this pull request Apr 1, 2023
Introduced by matplotlib#24691, merged shortly before matplotlib#24976

Not totally sold on the name 'RawColorType', but couldn't think of something I like better

Noticed that one of the other type aliases was not documented, so fixed that while I was editing these two files
ksunden added a commit to ksunden/matplotlib that referenced this pull request Apr 17, 2023
Introduced by matplotlib#24691, merged shortly before matplotlib#24976

Noticed that one of the other type aliases was not documented, so fixed that while I was editing these two files
eslothower pushed a commit to eslothower/matplotlib that referenced this pull request May 3, 2023
Introduced by matplotlib#24691, merged shortly before matplotlib#24976

Noticed that one of the other type aliases was not documented, so fixed that while I was editing these two files
@pierre-haessig
Copy link
Contributor

I just landed on the new example Ways to set a color's alpha value (which is really well indexed by search engines!). This new syntax seems pretty handy!

Now here is my problem: I didn't know this was a new example/feature until I realized in didn't work on my setup. I don't know what is the policy for marking new features in examples, but perhaps the "version added" should be explicit in examples?

@oscargus
Copy link
Member

Good point! We have from 3.7 tried to make it clear in the API documentation when things was added, but here it could clearly make sense to add a note in https://matplotlib.org/devdocs//users/explain/colors/colors.html#sphx-glr-users-explain-colors-colors-py that the last format is only available since 3.8. For individual examples, I am a bit more ambiguous (although I wouldn't block a PR with that). Not sure one can expect all to include it in a sensible way and at some stage one may want to expire that information in examples.

It is a bit annoying though that the search engines are pointing to the the development docs. We used to have the opposite problem, that the hits were from 2.x...

@pierre-haessig
Copy link
Contributor

Good point! We have from 3.7 tried to make it clear in the API documentation when things was added, but here it could clearly make sense to add a note in https://matplotlib.org/devdocs//users/explain/colors/colors.html#sphx-glr-users-explain-colors-colors-py that the last format is only available since 3.8. For individual examples, I am a bit more ambiguous (although I wouldn't block a PR with that). Not sure one can expect all to include it in a sensible way and at some stage one may want to expire that information in examples.

Interesting: I thought the other way round: put the version added to examples, because there is ample space to do that, while avoiding the clutter in the doc for color specification. In the end, there is still the fact that docs have a global version number.

And I agree along with a "version added policy" should come a sensible expiration policy (I thinking here of some Scipy function I saw the other day which was quite cluttered by old "version added" marks)

It is a bit annoying though that the search engines are pointing to the the development docs. We used to have the opposite problem, that the hits were from 2.x...

I probably landed from Qwant, so it's probably not a massive issue.

@oscargus
Copy link
Member

because there is ample space to do that, while avoiding the clutter in the doc for color specification.

Fair point. On the other hand the color specification is sort of "the official color specification API doc", so should be there, although it doesn't render that nicely.

I think my main "objection" is that I cannot really figure out a good way to get it into the example...

"""
In some cases, the (matplotlib_color, alpha) color format, introduced in 3.8, provides an easy way to fine-tune the appearance of a Figure.
"""
?

Or a

.. versionadded:: 3.8 
   The (matplotlib_color, alpha) color format.

at the start of the example?

@QuLogic
Copy link
Member

QuLogic commented Jun 28, 2023

We used to have a banner on devdocs; this was lost with the change to pydata-sphinx-theme. They do have an announcement banner now, though it may not work exactly the same. I am investigating if we can restore that functionality.

@story645
Copy link
Member

I think my main "objection" is that I cannot really figure out a good way to get it into the example...

plan with the tags/gsod work is to start adding a tag for when a feature was added, possibly automagically

@j1642
Copy link
Contributor Author

j1642 commented Jul 2, 2023

Also, we might want to add /devdocs/ to the exclusion list in robots.txt, which would request that search engines not include links to the devdocs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH]: set facecolor and edgecolor alpha separately
9 participants