Fix inability to use empty markers in scatter #25593

maramiulescu · 2023-03-31T18:08:27Z

PR Summary

Fixes #24404

Modifies the scatter function to allow users to plot using empty 'o' markers without any additional code.

PR Checklist

Documentation and Tests

Has pytest style unit tests (and pytest passes)
[N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
[N/A] 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/
[N/A] 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

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. 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.

lib/matplotlib/axes/_axes.py

tacaswell · 2023-03-31T18:58:05Z

lib/matplotlib/axes/_axes.py

+        if (colors is None or edgecolors is None) and not mcolors.is_color_like(c)\
+                and not type(c) == np.ndarray:


(I'm going to get the indentation wrong in the GH UI)

Suggested change

if (colors is None or edgecolors is None) and not mcolors.is_color_like(c)\

and not type(c) == np.ndarray:

if ((colors is None or edgecolors is None) and not mcolors.is_color_like(c)

and not type(c) == np.ndarray):

we prefer using () for multi-line expressions over using line-continuation.

I say that and then see a whole bunch of it in the code right above this 🤦🏻

tacaswell · 2023-03-31T19:01:11Z

lib/matplotlib/tests/test_axes.py

@@ -2760,7 +2762,10 @@ def get_next_color():
    _, _, result_edgecolors = \
        mpl.axes.Axes._parse_scatter_color_args(
            c, edgecolors, kwargs, xsize=2, get_next_color_func=get_next_color)
-    assert result_edgecolors == expected_edgecolors
+    if type(expected_edgecolors) == np.ndarray:


isinstance here as well please.

tacaswell · 2023-03-31T19:09:42Z

Thank you for this work @maramiulescu ! I left a few minor style comments. I feel strongly about the isinstance changes, but am more 🤷🏻 on the line continuation as it passes our style checker.

Would you be willing to write a whats new for this? I think that this feature is best explained with an image and is impactful enough we want to be sure to highlight it.

maramiulescu · 2023-03-31T19:18:37Z

Thanks @tacaswell for reviewing! I'm working together with @ewpardijs on this, we will implement your feedback and write a what's new soon.

tacaswell · 2023-03-31T19:21:18Z

We recently widened our linewidth to 88 (from 80) so you can be a bit more relaxed on that front for any new code you add and code immediately around it, but please do not reflow all of the code in this file.

lib/matplotlib/axes/_axes.py

ksunden · 2023-03-31T21:31:31Z

@maramiulescu FYI, a slight tweak to how you word the PR description will automatically link the PR to close that issue:

https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

(e.g. saying "Fixes #XXXXX" instead of "Fix issue #XXXXX")

I was going to just link this one manually, but the search box is not letting me select #24404 for some reason...

oscargus · 2023-04-01T10:57:49Z

( I edited the description before seeing @ksunden's comment, so now it is linked...)

maramiulescu · 2023-04-01T11:24:40Z

( I edited the description before seeing @ksunden's comment, so now it is linked...)

I edited it to say "Fixes ..." too, but it didn't get linked. Could it be that only a maintainer can link a PR after it's been created?

jklymak · 2023-04-01T14:28:00Z

I don't think the linking in the message is restricted to maintainers.

I'd also suggest a better title and description. Even should I click through to the issue the number won't mean anything to me in a week, whereas knowing what it fixed will.

erinproc · 2023-04-20T18:03:06Z

Hi, I am a new contributor and I was working on this issue when the pull request was submitted. I would still like to contribute in some way if possible, I noticed when looking at the files changed there was a warning that "Added lines #L4623 - L4624 were not covered by tests". Is this something that I could add? If not, if there is any other way I could help contribute, please let me know as soon as possible!

oscargus · 2023-04-20T18:05:27Z

@erinproc please have a look at https://github.com/matplotlib/matplotlib/issues?q=is%3Aissue+is%3Aopen+label%3A%22Good+first+issue%22 especially those that does not have a PR associated with them. That is probably a better starting point. But, yes, you are correct that ideally all lines should be covered in a PR.

maramiulescu · 2023-04-20T18:35:05Z

I don't think the linking in the message is restricted to maintainers.

I'd also suggest a better title and description. Even should I click through to the issue the number won't mean anything to me in a week, whereas knowing what it fixed will.

Updated the title and the description

erinproc · 2023-04-20T18:38:31Z

@oscargus thank you for your advice. I was working on this issue for an assignment in my software engineering course which is due soon, so I don't really have time to pivot to a new issue, but I'd still like to contribute to this pull request if possible. I have written some new tests and I was wondering what is the best way to submit these to be added to the current pull request?

oscargus · 2023-04-20T18:43:02Z

@erinproc Wait until this is merged and then contribute them I'd say.

You can also add your commits on top of this, put that may still require rebasing later on so probably better to wait.

erinproc · 2023-04-20T18:47:46Z

@oscargus thanks for all of your help and advice, I really appreciate it. Out of curiosity, how long might it take to merge this pull request? I'm unfamiliar with the timeline of these types of things, and I'd just like to have a general idea.

oscargus · 2023-04-20T18:59:45Z

I'd say between one hour and, well, the oldest we have is six years... I'm not really up to date with this one, but hopefully shouldn't be long.

maramiulescu · 2023-04-20T20:31:55Z

Would you be willing to write a whats new for this? I think that this feature is best explained with an image and is impactful enough we want to be sure to highlight it.

@tacaswell What's new added, could you have a look?

lib/matplotlib/axes/_axes.py

ksunden · 2023-04-20T21:30:15Z

doc/users/next_whats_new/updated_scatter.rst

+Enable configuration of empty markers in `~matplotlib.axes.Axes.scatter`
+------------------------------------------------------------------------
+
+`~matplotlib.axes.Axes.scatter` can now be configured to plot empty markers without additional code. Setting ``facecolors`` to *'none'* and defining ``c`` now draws only the edge colors for fillable markers.


I think this should more explicitly highlight the "with colormap" part of this change, it has always been possible to have empty single color markers with facecolor='none', edgecolor='C0', e.g.

Admittedly it is a bit surprising that specifying edgecolor explicitly was required, but that is due to the default being to follow "face"

Does this PR do anything to make that behavior change (without a cmap?)

We've changed the text in 39cfd56, hopefully now it's more fitting. If I understand your question correctly, yes, this PR does change that behavior: for the value of edgecolor, c now takes precedence over face

ksunden · 2023-05-15T23:47:46Z

lib/matplotlib/axes/_axes.py

@@ -4338,6 +4340,7 @@ def _parse_scatter_color_args(c, edgecolors, kwargs, xsize,
        - kwargs['edgecolor']
        - edgecolors (is an explicit kw argument in scatter())
        - kwargs['color'] (==kwcolor)
+        - c (if not None)


I think this is only true if facecolors is "none". Otherwise I would expect the test case (dict(c='b'), None), from the edited test to instead be (dict(c='b'), np.array([[0, 0, 1, 1]])), similar to the last test case added... I think, at least?

tacaswell · 2024-03-13T21:28:24Z

lib/matplotlib/axes/_axes.py

+            x, y, s, linewidths = \
+                cbook._combine_masks(x, y, s, linewidths)


Suggested change

x, y, s, linewidths = \

cbook._combine_masks(x, y, s, linewidths)

x, y, s, linewidths = cbook._combine_masks(x, y, s, linewidths)

tacaswell · 2024-03-13T21:28:49Z

lib/matplotlib/axes/_axes.py

+            c = np.ma.masked_invalid(c)
+            x, y, s, colors, linewidths = \
+                cbook._combine_masks(x, y, s, colors, linewidths)


Suggested change

c = np.ma.masked_invalid(c)

x, y, s, colors, linewidths = \

cbook._combine_masks(x, y, s, colors, linewidths)

c = np.ma.masked_invalid(c)

x, y, s, colors, linewidths = cbook._combine_masks(x, y, s, colors, linewidths)

This also needs a test case that will cover this line.

tacaswell · 2024-03-13T21:30:52Z

lib/matplotlib/axes/_axes.py

-        if colors is None:
+
+        if ((colors is None or edgecolors is None) and not mcolors.is_color_like(c) and
+                (not isinstance(c, np.ndarray) or isinstance(c, np.ma.MaskedArray))):


Why are we gating on the type here? Will our normalization above ensure other iterables will be converted to numpy arrays?

tacaswell · 2024-03-13T21:35:55Z

lib/matplotlib/tests/test_axes.py

@@ -2749,6 +2749,8 @@ def get_next_color():
     (dict(c='b', edgecolor='r', edgecolors='g'), 'r'),
     (dict(color='r'), 'r'),
     (dict(color='r', edgecolor='g'), 'g'),
+     (dict(facecolors='none'), None),
+     (dict(c='b', facecolors='none'), np.array([[0, 0, 1, 1]]))


I think we need to cover the case of colormapping + empty here as well.

tacaswell · 2024-03-13T21:36:41Z

@maramiulescu Sorry this fell off our radar.

claytharrison · 2025-02-19T13:46:18Z

I'd be interested in taking this over if @maramiulescu has no objections. It's been bugging me for ages and it would be nice to just have it finally work.

maramiulescu · 2025-02-19T13:50:27Z

It would be great if you could take over @claytharrison

maramiulescu added 3 commits March 31, 2023 20:04

Fix trailing whitespace

0d8bfed

Update scatter test to work with new behavior

a0662df

Add tests

d065990

github-actions bot reviewed Mar 31, 2023

View reviewed changes

tacaswell added this to the v3.8.0 milestone Mar 31, 2023

tacaswell reviewed Mar 31, 2023

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

tacaswell reviewed Mar 31, 2023

View reviewed changes

Changed typechecks to isinstance, line-continuations to brackets

eb97647

ksunden reviewed Mar 31, 2023

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

Merge branch 'matplotlib:main' into fix-issue-24404

aa6e01e

Merge branch 'matplotlib:main' into fix-issue-24404

ae488b3

maramiulescu changed the title ~~Fix issue #24404~~ Fix inability to use empty markers in scatter Apr 20, 2023

maramiulescu and others added 3 commits April 20, 2023 21:22

Use isinstance for type checking

c8d7ff3

Add typecheck for maskedarrays

0072668

Add what's new

679603a

ksunden reviewed Apr 20, 2023

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

ksunden reviewed Apr 20, 2023

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

ksunden reviewed Apr 20, 2023

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

ksunden reviewed Apr 20, 2023

View reviewed changes

PR comments and updated whats new

39cfd56

ksunden mentioned this pull request Apr 25, 2023

Scatter plot markers disappear completely when using facecolors='none' #25764

Closed

ewpardijs and others added 2 commits May 5, 2023 22:00

Merge branch 'matplotlib:main' into fix-issue-24404

4bdcd3c

Merge branch 'matplotlib:main' into fix-issue-24404

28f0692

ksunden reviewed May 15, 2023

View reviewed changes

jklymak added the status: needs revision label Jun 13, 2023

ksunden modified the milestones: v3.8.0, v3.9.0 Aug 8, 2023

tacaswell reviewed Mar 13, 2024

View reviewed changes

QuLogic modified the milestones: v3.9.0, v3.10.0 Mar 27, 2024

ksunden modified the milestones: v3.10.0, future releases Oct 9, 2024

github-actions bot added the status: needs rebase label Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inability to use empty markers in scatter #25593

Fix inability to use empty markers in scatter #25593

maramiulescu commented Mar 31, 2023 •

edited

Loading

github-actions bot left a comment

tacaswell Mar 31, 2023

tacaswell Mar 31, 2023

maramiulescu Apr 22, 2023

tacaswell Mar 31, 2023

maramiulescu Apr 22, 2023

tacaswell commented Mar 31, 2023

maramiulescu commented Mar 31, 2023

tacaswell commented Mar 31, 2023

ksunden commented Mar 31, 2023

oscargus commented Apr 1, 2023

maramiulescu commented Apr 1, 2023

jklymak commented Apr 1, 2023 •

edited

Loading

erinproc commented Apr 20, 2023

oscargus commented Apr 20, 2023

maramiulescu commented Apr 20, 2023

erinproc commented Apr 20, 2023

oscargus commented Apr 20, 2023

erinproc commented Apr 20, 2023

oscargus commented Apr 20, 2023

maramiulescu commented Apr 20, 2023 •

edited

Loading

ksunden Apr 20, 2023

maramiulescu Apr 21, 2023

ksunden May 15, 2023

tacaswell Mar 13, 2024

tacaswell Mar 13, 2024

tacaswell Mar 13, 2024

tacaswell Mar 13, 2024

tacaswell Mar 13, 2024

tacaswell commented Mar 13, 2024

claytharrison commented Feb 19, 2025

maramiulescu commented Feb 19, 2025

		if (colors is None or edgecolors is None) and not mcolors.is_color_like(c)\
		and not type(c) == np.ndarray:

		x, y, s, linewidths = \
		cbook._combine_masks(x, y, s, linewidths)

	x, y, s, linewidths = \
	cbook._combine_masks(x, y, s, linewidths)
	x, y, s, linewidths = cbook._combine_masks(x, y, s, linewidths)

Fix inability to use empty markers in scatter #25593

Are you sure you want to change the base?

Fix inability to use empty markers in scatter #25593

Conversation

maramiulescu commented Mar 31, 2023 • edited Loading

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Mar 31, 2023

maramiulescu commented Mar 31, 2023

tacaswell commented Mar 31, 2023

ksunden commented Mar 31, 2023

oscargus commented Apr 1, 2023

maramiulescu commented Apr 1, 2023

jklymak commented Apr 1, 2023 • edited Loading

erinproc commented Apr 20, 2023

oscargus commented Apr 20, 2023

maramiulescu commented Apr 20, 2023

erinproc commented Apr 20, 2023

oscargus commented Apr 20, 2023

erinproc commented Apr 20, 2023

oscargus commented Apr 20, 2023

maramiulescu commented Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Mar 13, 2024

claytharrison commented Feb 19, 2025

maramiulescu commented Feb 19, 2025

maramiulescu commented Mar 31, 2023 •

edited

Loading

jklymak commented Apr 1, 2023 •

edited

Loading

maramiulescu commented Apr 20, 2023 •

edited

Loading