Make sure custom alpha param does not change 'none' colors in a list of colors #27845
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
timhoffm
reviewed
Mar 3, 2024
lib/matplotlib/tests/test_colors.py
Outdated
| @@ -1660,3 +1663,25 @@ def test_set_cmap_mismatched_name(): | |||
| cmap_returned = plt.get_cmap("wrong-cmap") | |||
| assert cmap_returned == cmap | |||
| assert cmap_returned.name == "wrong-cmap" | |||
|
|
|||
|
|
|||
| def test_pathcollection_alpha_none_facecolor(): | |||
There was a problem hiding this comment.
Please run the test on to_rgba_array() directly, not on PathCollection. While you've experienced the problem in PatchCollection, the actual fault and behavior change is in that function. It's better to keep the scope of unit tests very focussed.
There was a problem hiding this comment.
Sure, thanks. I've rewritten the unit test to test specifically the changed code in to_rgba_array. Please let me know if there are any problems.
Plus, I am seeing building check failures due to subprocess timeouts which does not seem to be related to my code change. How should I fix this?
chaoyihu
force-pushed
the
bugfix-issue-27839
branch
from
c814ffd
to
67e7fb2
Compare
timhoffm
reviewed
Mar 5, 2024
chaoyihu
force-pushed
the
bugfix-issue-27839
branch
from
77e5871
to
94b620c
Compare
|
Something went wrong with the most recent commit it contains the plot_date deprecation, which is already on main. I suggest you squash and rebase on main. |
chaoyihu
force-pushed
the
bugfix-issue-27839
branch
from
94b620c
to
22101f6
Compare
Apply review feedbacks Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
chaoyihu
force-pushed
the
bugfix-issue-27839
branch
from
22101f6
to
c3b182e
Compare
|
Sorry, I think it's because I synced fork by mistake. I made a new commit and the code changes are correct now. Also squashed the commits as suggested. |
timhoffm
approved these changes
Mar 6, 2024
|
CI failure is unrelated. I'm milestoning as 3.9 because I'd like to see this in there and the fix is quick and easy to review. |
QuLogic
reviewed
Mar 8, 2024
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
QuLogic
approved these changes
Mar 9, 2024
|
Thanks @chaoyihu! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again. |
Impaler343
pushed a commit
to Impaler343/matplotlib
that referenced
this pull request
Mar 14, 2024
…of colors (matplotlib#27845) * fix issue 27839, make sure alpha param does not affect none colors Apply review feedbacks Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> * Update lib/matplotlib/colors.py Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> --------- Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Impaler343
added a commit
to Impaler343/matplotlib
that referenced
this pull request
Mar 14, 2024
pin pytest Deprecate plot_date Correctly set temporary pdf/pgf backends Calling `FigureCanvasPdf(figure)` will call `figure.set_canvas(self)`, meaning the `cbook._setattr_cm` context manager that wraps this change will see the _new_ canvas, and the old canvas isn't restored. Instead, just pass the `backend` parameter, as `savefig` already knows how to correctly save and restore the canvas if it needs to change backends. Fixes matplotlib#27865 Bump the actions group with 2 updates Bumps the actions group with 2 updates: [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish) and [scientific-python/upload-nightly-action](https://github.com/scientific-python/upload-nightly-action). Updates `pypa/gh-action-pypi-publish` from 1.8.11 to 1.8.12 - [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases) - [Commits](pypa/gh-action-pypi-publish@2f6f737...e53eb8b) Updates `scientific-python/upload-nightly-action` from 0.3.0 to 0.5.0 - [Release notes](https://github.com/scientific-python/upload-nightly-action/releases) - [Commits](scientific-python/upload-nightly-action@6e9304f...b67d7fc) --- updated-dependencies: - dependency-name: pypa/gh-action-pypi-publish dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions - dependency-name: scientific-python/upload-nightly-action dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions ... Signed-off-by: dependabot[bot] <support@github.com> Bump pydata-sphinx-theme to 0.15 Fix search button placement in navbar Fix doc sidebar Fix version switcher url for circleCI Don't use custom CSS for announcement banner Disable parallel build Don't show doc source link Add dtype/copy args to internal testing class Use pybind11 string formatter for exception messages This can eventually be replaced by `std::format` when we require C++20. doc: add description of **kwargs usage to collections (matplotlib#27872) * doc: add description of **kwargs usage to collections * Update lib/matplotlib/collections.py Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> --------- Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> TST: adding tests of current clear behavior on ticks closes matplotlib#23839 DOC: update comments and docstrings Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> Update lib/matplotlib/tests/test_axes.py BLD,Cygwin: Included Python.h first in src/_c_internal_utils.cpp Python.h needs to be included before any system includes. This is easiest if it is the first include. Interestingly, pybind11/pybind11.h does not include Python, possibly depending on downstream projects including Python.h beforehand. CI: Include Python.h first in _tkagg.cpp Needs to be included before system headers to set visibility macros for pybind11 BLD: Include Python.h first in _backend_agg.cpp BLD: Include Python.h before pybind11/pybind11.h pybind11 assumes Python.h is included first. CI: Specify python version for default pytest. CI,Cygwin: Run pytest with python -m pytest Cygwin pytest recently updated, and only installs pytest-3.9 CI,Cygwin: Revert use of alternatives to set pytest script. Cygwin pytest only installs pytest-3.9. I should possibly suggest using alternatives for that and pip. CI,Cygwin: Avoid running pytest in root directory. CI,Cygwin: Revert running pytest in different directory. CI,Cygwin: Call pytest-3.9 explicitly. BLD: Include Python.h first in src/_path.h BLD,BUG: Remove Python.h includes immediately preceeding pybind11/pybind11.h includes pybind11 seems decent about including Python.h before any system headers, once you chase three layers of includes in to see that. FIX: handle nans in RGBA input with ScalarMappables DOC: Update some animation related topics - use `set_data_3d` - cross-reference `Line2d` `set_data`, `set_xdata`, `set_ydata` - Rewrite parts of the FuncAnimation description Inspired through matplotlib#27830. Revert "Merge branch 'matplotlib:main' into doc-change" This reverts commit feeabe6, reversing changes made to bcbc2ef. Reapply "Merge branch 'matplotlib:main' into doc-change" This reverts commit ed91cee. Convert path extension to pybind11 Add a pybind11 type caster for agg::rect_d Add a pybind11 type caster for agg::trans_affine Add a pybind11 type caster for mpl::PathIterator Add a pybind11 type caster for e_snap_mode Add a pybind11 type caster for SketchParams Optimize convert_polygon_vector a bit FIX: don't copy twice on RGB input Fix devdocs version switcher Allow passing a transformation to secondary_[xy]axis Add transform argument to secondary axes Update _secax_docstring Move new params to end of functions Add input check to secondary axes Add tests Add examples Move transform type checks and improve docs Add type stubs Update _secax_docstring Move new params to end of functions Add input check to secondary axes Move transform type checks and improve docs Fix rebase error Fix stub for SecondaryAxis.__init__ Clarify example Add default param to secax constructor Fix stub for secax constructor Simplify imports Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> Remove redundancy in docs Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> Fix typo DOC: fix stray release note entry BLD: Add a fallback URL for FreeType FreeType is available from both Savannah and SourceForge, and sometimes AppVeyor seems to have trouble downloading from Savannah, so perhaps this fallback will help. We used to try both these URLs in the pre-Meson build system. DOC: State approximate documentation build time Since the doc build takes very long, an order-of-magnitude estimate is helpful to manage expectations. I'm not wedded to the actual numbers, if somebody has better suggestions. A quick test on an old i7 6700 (from 2015) yielded - `O=-j1`: 23min - `O=-j4`: 15min Doc build on CI is around 18min. Add BackendRegistry singleton class Use backend_registry for name of singleton instance Use class variables for immutable collections Review comments Update lib/matplotlib/backend_bases.py Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> Linting/mypy fixes Remove INTERACTIVE_NON_WEB backend filter Small changes from review Use _api.caching_module_getattr for deprecated module-level attributes Add docstrings Add api changes and what new docs Fix docs Inline the deprecation function calls Import BackendFilter and backend_registry into matplotlib.backends.__init__.py Mypy fixes Remove unused _safe_pyplot_import Remove unneeded type annotations Make sure custom alpha param does not change 'none' colors in a list of colors (matplotlib#27845) * fix issue 27839, make sure alpha param does not affect none colors Apply review feedbacks Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> * Update lib/matplotlib/colors.py Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> --------- Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com> Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> DOC: fix :mpltype:`color` role `inline.interpreted()` already returns a list of nodes. We mustn't wrap it in another list. Use :mpltype:`color` for color types
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR summary
This change suggests a fix and hopefully closes #27839 by making sure that 'none' colors in a list won't be affected by custom alpha param.
Before the fix, this code:
will change rgba array of color list ['blue', 'none'] from:
to
After the fix, any 'none' color in a list of colors will be kept colorless.
A test is added to make sure the function works as expected.
PR checklist