Skip to content

Require non-zero dash value #22569

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 1 commit into from
Mar 3, 2022
Merged

Conversation

oscargus
Copy link
Member

PR Summary

In #14498 (comment) it is mentioned that an all zero dashes hangs.

This PR introduces an exception for this case.

One may consider changing

if np.any(dl < 0.0):
to use <= instead. It would be consistent with the error message and as far as I can tell, 0 dash values do not affect anything. But it may break stuff, I assume.

I did not manage to test this properly. Code that generates exceptions in the shell does not do it in the tests. Not sure why.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@oscargus
Copy link
Member Author

Maybe one would also like to validate the rcParams like this? No negative and at least one non-zero? Or simply go with all positive (no zeros)?

@anntzer
Copy link
Contributor

anntzer commented Feb 28, 2022

We can of course decide on whatever behavior we want, but note that at least cairo and svg allow zero-length "on" segments, as these will draw squares if the linecap is square and circles if the linecap is round (https://cairographics.org/manual/cairo-cairo-t.html#cairo-set-dash, https://svgwg.org/svg2-draft/paths.html#ZeroLengthSegments).

@oscargus
Copy link
Member Author

OK! Then I think it makes sense to still allow it (even without your insight, it can still break existing scripts if we disallow it).

@oscargus oscargus force-pushed the nonzerodashes branch 2 times, most recently from 11842f7 to 463cd8a Compare February 28, 2022 11:54
@tacaswell
Copy link
Member

I think the right check is that the sequence seeds to sum to 0. I suspect the hang reported by that user is because we are trying to draw a line, but only taking 0 sized steps. Having at least 1 non-zero step would fix that.

@tacaswell tacaswell added this to the v3.6.0 milestone Mar 1, 2022
@oscargus
Copy link
Member Author

oscargus commented Mar 2, 2022

So negative steps are OK? If not, I do not really see the difference between checking for at least one positive value and sum != 0.

@timhoffm
Copy link
Member

timhoffm commented Mar 2, 2022

At least Cairo does not allow negative segments (https://cairographics.org/manual/cairo-cairo-t.html#cairo-set-dash). I suppose if they don't have it, we don't need it either.

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.

4 participants