Skip to content

Enh improve agg chunks error #19343

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 6 commits into from
Sep 25, 2021

Conversation

tacaswell
Copy link
Member

PR Summary

This does 2 important things:

  • makes the errors when you exceed the chunk size more instructive of how to fix your problem
  • makes the upper limit higher so it will be hit less often

and 2 less important things:

  • makes a test more pathological
  • makes sure the simplification threshold is propagated when chunking.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).

@tacaswell tacaswell added this to the v3.5.0 milestone Jan 23, 2021
@jklymak jklymak marked this pull request as draft May 10, 2021 16:29
@tacaswell tacaswell force-pushed the enh_improve_agg_chunks_error branch from 150617a to 6cfa6ed Compare May 27, 2021 00:06
@tacaswell tacaswell marked this pull request as ready for review May 27, 2021 00:06
@tacaswell tacaswell force-pushed the enh_improve_agg_chunks_error branch from c01a41d to 52f7610 Compare May 27, 2021 00:19
@jklymak jklymak marked this pull request as draft June 8, 2021 21:03
@tacaswell tacaswell force-pushed the enh_improve_agg_chunks_error branch from 52f7610 to c19d49f Compare August 5, 2021 18:03
@tacaswell tacaswell marked this pull request as ready for review August 5, 2021 18:03
@tacaswell tacaswell marked this pull request as draft August 5, 2021 23:54
@tacaswell tacaswell force-pushed the enh_improve_agg_chunks_error branch from c19d49f to 51ae0a8 Compare September 8, 2021 20:17
@tacaswell tacaswell marked this pull request as ready for review September 16, 2021 19:25
tacaswell and others added 5 commits September 22, 2021 15:19
The error messages now provide better instruction on how to fix the
problem.

There are some paths we can not split (one with hatching, fills, or
that claim they can not be simplified) so tell user that.

If we could have otherwise split up the path if the chunksize is set
to less than 100, which disables the feature, ask user to make it
bigger or make the path simplification more aggressive.

If the chunksize is bigger than the data, ask user to make it smaller
or make the path simplification more aggressive.

If the chunksize is smaller than the data, but still too big ask user
to make it smaller or make the path simplification more aggressive.

closes matplotlib#19325

Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
This allows longer, more textured, paths to be drawn with the default
settings at the risk of very long run times.
- Make sure values are be big enough / small enough
- make long path test more pathological (Random data will sometimes go in the
  same direction)
@tacaswell tacaswell force-pushed the enh_improve_agg_chunks_error branch from 51ae0a8 to 09fa321 Compare September 22, 2021 19:19
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

I've had a look over and it looks good - there's one change I don't understand at all though (left a comment) so I'm not personally comfortable approving yet.

@@ -45,7 +45,7 @@ RendererAgg::RendererAgg(unsigned int width, unsigned int height, double dpi)
rendererBase(),
rendererAA(),
rendererBin(),
theRasterizer(8192),
theRasterizer(32768),
Copy link
Member

Choose a reason for hiding this comment

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

What's the meaning of this change?

Copy link
Member

Choose a reason for hiding this comment

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

And does it need documenting in an API change note?

Copy link
Member

Choose a reason for hiding this comment

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

That is:

  • makes the upper limit higher so it will be hit less often

@QuLogic QuLogic merged commit 1357c52 into matplotlib:master Sep 25, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 25, 2021
QuLogic added a commit that referenced this pull request Sep 25, 2021
…343-on-v3.5.x

Backport PR #19343 on branch v3.5.x (Enh improve agg chunks error)
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Oct 12, 2021
tacaswell pushed a commit that referenced this pull request Oct 20, 2021
@tacaswell tacaswell deleted the enh_improve_agg_chunks_error branch November 30, 2021 04:11
@tacaswell tacaswell mentioned this pull request Dec 5, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants