Raise on plural scatter #25794
Raise on plural scatter #25794
Conversation
|
Thank you for your work on this, however this logic should live in We already have some logic for resolving these aliases, I am a bit surprised that we did not already handle this case. |
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 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.
|
Hi @tacaswell I've moved what you asked, and I also moved the tests to the correct test file |
|
@MichaelTheFear did you forget to push the tests? |
|
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? |
lib/matplotlib/tests/test_axes.py
Outdated
| 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"]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sorry that I didn't see this earlier.
There was a problem hiding this comment.
No problem. Should I remove the asserts?
|
Both of those look like network issues on the CI side. We take the alias This is a common enough problem we do have some machinery to automate it (e.g. matplotlib/lib/matplotlib/artist.py Lines 1222 to 1226 in e7fd79f scatter.
Rather than hard-coding the confilcts, we should at least pull from matplotlib/lib/matplotlib/collections.py Lines 27 to 35 in e7fd79f 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! |
|
Thank you. @tacaswell Is there anything else that I could do about this PR? |
|
I think we'd want to be using |
|
Sorry about taking so long to resolve this, I've made those requested changes of using matplotlib/lib/matplotlib/axes/_axes.py Lines 4555 to 4562 in 63eae54 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 matplotlib/lib/matplotlib/axes/_axes.py Lines 4562 to 4566 in 63eae54 matplotlib/lib/matplotlib/axes/_axes.py Line 4688 in 63eae54 |
|
This looks good to me! Would you be willing / able to squash this to one commit? |
|
@tacaswell Done 👍 |
|
Thanks and congratulations on your first contribution to Matplotlib! We'd be happy to see you again. |
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