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

Raise on plural scatter #25794

Merged
merged 1 commit into from May 26, 2023
Merged

Raise on plural scatter #25794

merged 1 commit into from May 26, 2023

Conversation

MichaelTheFear
Copy link

PR summary

Closes #19120

I've created an exception when you pass both singular and plural arguments of pyplot.scatter for edgecolor(s) and linewidth(s), I also created test suits for the exception.

PR checklist

@tacaswell
Copy link
Member

Thank you for your work on this, however this logic should live in Axes.scatter not in pyplot. Could you move it there instead?

We already have some logic for resolving these aliases, I am a bit surprised that we did not already handle this case.

@tacaswell tacaswell added this to the v3.8.0 milestone May 1, 2023
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.

This should be handled in Axes.scatter not in the pyplot wrapper.

Anyone can clear this review when it is addressed.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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.

@MichaelTheFear
Copy link
Author

Hi @tacaswell I've moved what you asked, and I also moved the tests to the correct test file

@timhoffm
Copy link
Member

timhoffm commented May 1, 2023

@MichaelTheFear did you forget to push the tests?

@MichaelTheFear
Copy link
Author

Hi! I couldn't understand why some checks were not successful. The fact that they were dependency errors isn't really clear to me. Does anyone know why and how to fix it?

Comment on lines 2717 to 2720
assert plt.scatter([1, 2, 3], [1, 2, 3], linewidths=[0.5, 0.4, 0.3],
edgecolor="#ffffff")
assert plt.scatter([1, 2, 3], [1, 2, 3], linewidth=0.3,
edgecolors=["#ffffff", "#000000", "#f0f0f0"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure these are required as they basically checks that there are no errors in these cases? Or does it complain about not having full coverage if these are not there?

I think the errors now has nothing to do with your PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry that I didn't see this earlier.

Copy link
Author

Choose a reason for hiding this comment

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

No problem. Should I remove the asserts?

tacaswell
tacaswell previously approved these changes May 1, 2023
@tacaswell tacaswell dismissed their stale review May 1, 2023 16:38

checks are moved.

@tacaswell
Copy link
Member

Both of those look like network issues on the CI side.

We take the alias lw as well which I think will still sneak through.

This is a common enough problem we do have some machinery to automate it (e.g.

def set(self, **kwargs):
# docstring and signature are auto-generated via
# Artist._update_set_signature_and_docstring() at the end of the
# module.
return self._internal_update(cbook.normalize_kwargs(kwargs, self))
) however it seems that we do not use any of that machinery in scatter.

Rather than hard-coding the confilcts, we should at least pull from

@_api.define_aliases({
"antialiased": ["antialiaseds", "aa"],
"edgecolor": ["edgecolors", "ec"],
"facecolor": ["facecolors", "fc"],
"linestyle": ["linestyles", "dashes", "ls"],
"linewidth": ["linewidths", "lw"],
"offset_transform": ["transOffset"],
})
class Collection(artist.Artist, cm.ScalarMappable):
.

I'm not clear if this is going to be a simple change or if starting to pull at this is going to keep unraveling!

@MichaelTheFear
Copy link
Author

Thank you. @tacaswell Is there anything else that I could do about this PR?

@QuLogic
Copy link
Member

QuLogic commented May 3, 2023

I think we'd want to be using cbook.normalize_kwargs here instead of manually checking and raising; the question is whether it should be applied to just the two options here or everything that a Collection aliases as linked by @tacaswell.

@MichaelTheFear MichaelTheFear marked this pull request as draft May 19, 2023 18:34
@MichaelTheFear MichaelTheFear marked this pull request as ready for review May 24, 2023 19:27
@MichaelTheFear
Copy link
Author

Sorry about taking so long to resolve this, I've made those requested changes of using cbook.normalize_kwargs, and using the collections aliases

# add edgecolors and linewidths to kwargs so they
# can be processed by normailze_kwargs
if edgecolors is not None:
kwargs.update({'edgecolors': edgecolors})
if linewidths is not None:
kwargs.update({'linewidths': linewidths})
kwargs = cbook.normalize_kwargs(kwargs, mcoll.Collection)

and I also had to assign them back to its original variables right after because it was causing and error mainly with linewidths on the cbook._intenal_update

kwargs = cbook.normalize_kwargs(kwargs, mcoll.Collection)
# re direct linewidth and edgecolor so it can be
# further processed by the rest of the function
linewidths = kwargs.pop('linewidth', None)
edgecolors = kwargs.pop('edgecolor', None)

collection._internal_update(kwargs)

@tacaswell
Copy link
Member

This looks good to me!

Would you be willing / able to squash this to one commit?

@MichaelTheFear
Copy link
Author

@tacaswell Done 👍

@timhoffm timhoffm merged commit f1e0274 into matplotlib:main May 26, 2023
37 of 39 checks passed
@timhoffm
Copy link
Member

Thanks and congratulations on your first contribution to Matplotlib! We'd be happy to see you again.

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

Successfully merging this pull request may close these issues.

Raise when both singular and plural scatter attributes are specified
5 participants