8000 Simplify `colored_line()` implementation in Multicolored lines example by 34j · Pull Request #29954 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Simplify colored_line() implementation in Multicolored lines example #29954

8000 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 3 commits into from
Apr 22, 2025

Conversation

34j
Copy link
Contributor
@34j 34j commented Apr 21, 2025

PR summary

  • Reduces the number of lines.
  • Stops using Numpy functions which is not array-api compatible (hstack, column_stack, etc.), which improves readability.
  • Calls autoscale_view, thus set_xlim, etc. need not to be called manually.
  • Adds stacklevel=2 in warnings.warn.
  • (Note: I have created a dedicated package for this function: https://github.com/34j/matplotlib-multicolored-line)

PR checklist

@github-actions github-actions bot added the Documentation: examples files in galleries/examples label Apr 21, 2025
@34j 34j force-pushed the perf/better-multicolored-line branch from 39d5328 to 47e4a69 Compare April 21, 2025 12:23
@34j 34j changed the title refactor: simplify colored_line() implementation in Multicolored lines example Simplify colored_line() implementation in Multicolored lines example Apr 21, 2025
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 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.

(xy[0, :][None, :], (xy[:-1, :] + xy[1:, :]) / 2, xy[-1, :][None, :]), axis=0
)
segments = np.stack((xy_mid[:-1, :], xy, xy_mid[1:, :]), axis=-2)
# Note that segments is [
Copy link
Member

Choose a reason for hiding this comment

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

This comment is really confusing, and I don't think is correct, but certainly possible I am just confused. Segments should each be [0.5 * (xy[j-1] + xy[j]), xy[j], 0.5 * (xy[j] + xy[j+1])]?

Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

If something is "array" compatible, doesn't "np.asarray" convert it? I'm not clear on the motivation for this suggested change. I suppose it is a touch more efficient?

@timhoffm
Copy link
Member

If something is "array" compatible, doesn't "np.asarray" convert it? I'm not clear on the motivation for this suggested change. I suppose it is a touch more efficient?

Since the following code still uses numpy methods, the only change here is for subclasses of ndarray, which np.asarray converts to np.array. I suspect the intention is also to make it more array-api-compatible, but I would not jump extra hoops for this here because this is an example. As such it's primary goal is to be instructive, not complete - Disclaimer: I haven't checked the numeric changes. So, can't judge wether the code gets simpler or not.

@timhoffm timhoffm added this to the v3.11.0 milestone Apr 22, 2025
@timhoffm timhoffm merged commit 697c240 into matplotlib:main Apr 22, 2025
22 checks passed
@34j 34j deleted the perf/better-multicolored-line branch April 23, 2025 00:13
@34j
Copy link
Contributor Author
34j commented Apr 23, 2025

I have noticed that making the starting point of each segment of the LineCollection more forward would make it plot more beautifully. I don't want to send any more PRs because there are seemingly endless improvements to this function, but I would be glad to have it as a consideration.

https://github.com/34j/matplotlib-multicolored-line/blob/dd67d48d315ea291412a5b566fb5507b08647b65/src/matplotlib_multicolored_line/_main.py#L79-L87

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: examples files in galleries/examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0