FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix issue with sketch not working on PathCollection in Agg by oscargus · Pull Request #25645 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Fix issue with sketch not working on PathCollection in Agg - #25645

Merged
QuLogic merged 1 commit into
matplotlib:mainfrom
oscargus:aggsketchcollection
Sep 26, 2024
ConversationCommits (1)ChecksFiles changed
Merged

Fix issue with sketch not working on PathCollection in Agg#25645
QuLogic merged 1 commit into
matplotlib:mainfrom
oscargus:aggsketchcollection

Conversation

oscargus commented Apr 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

Closes #19693

Seems like sketch was not handled at all for path collections.

Maybe not the nicest code, but my C/C++ is not that good.

If I understand it correctly, sketch isn't really tested (as it is hard to recreate the images?)?

Here is the output anyway:

PR Checklist

Documentation and Tests

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

oscargus added this to the v3.8.0 milestone Apr 7, 2023
oscargus force-pushed the aggsketchcollection branch from 49b3d47 to b235cbb Compare April 7, 2023 12:43

story645 commented Apr 9, 2023

Copy link
Copy Markdown
Member

Can you try an image test? I think they're recreatable if you use a fixed seed?

oscargus commented Apr 10, 2023

Copy link
Copy Markdown
Member Author

We can only control the seed for data generation in NumPy, not the seed for the C++-part.

However, there is #13479 which is related.

story645 commented Apr 20, 2023

Copy link
Copy Markdown
Member

yeah, I agree with Jody that #13479 (comment) is the correct solution (and also probably out of scope here)

oscargus commented Apr 27, 2023

Copy link
Copy Markdown
Member Author

I've realized that sketching is in general not tested that well (if at all). However, although the result is (probably) deterministic at the moment, I think that it is better to add tests as part of implementing seeding.

story645 mentioned this pull request Jul 11, 2023
Open
6 tasks
ksunden modified the milestones: v3.8.0, v3.9.0 Aug 8, 2023
oscargus mentioned this pull request Nov 13, 2023
Merged
1 task done
QuLogic modified the milestones: v3.9.0, v3.10.0 Apr 3, 2024
QuLogic approved these changes Sep 14, 2024
View reviewed changes

QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks reasonable, but I guess it needs rebase to refresh CI?

timhoffm commented Sep 20, 2024

Copy link
Copy Markdown
Member

@oscargus please rebase

oscargus commented Sep 20, 2024

Copy link
Copy Markdown
Member Author

@oscargus please rebase

Will sort it out over the weekend.

oscargus commented Sep 26, 2024

Copy link
Copy Markdown
Member Author

No conflicts during rebase (but apparently a very long/short weekend...).

QuLogic merged commit 49eeefc into matplotlib:main Sep 26, 2024
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

QuLogic QuLogic approved these changes
timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

backend: agg topic: path handling

Projects

None yet

Milestone

v3.10.0

Development

Successfully merging this pull request may close these issues.

path.sketch doesn't apply to PolyCollection

5 participants

Back | FazBrowse Home | New Git URL