Use cycling iterators in RendererBase. #21696
Conversation
| gc0.set_foreground(fg) | ||
| gc0.set_dashes(*ls) | ||
| if len(ec) == 4 and ec[3] == 0.0: | ||
| gc0.set_linewidth(0) |
There was a problem hiding this comment.
This seems like an important special case to have a test for? If I am reading this right, this is a fast-path to set a line with 0 alpha to 0 width?
There was a problem hiding this comment.
I think it's just an optimization? In fact, backend_ps (the only backend with no support for transparency, AFAICT) already special-cases transparent lines to not be drawn in its own implementation of draw_path_collection (via _draw_ps), so I'd say we could even get rid of this special case (well, in this PR I was just keeping the old semantics, but if you're up for changing that as well...).
lib/matplotlib/backend_bases.py
Outdated
| trfs = (itertools.cycle(map(Affine2D, all_transforms)) if Ntransforms | ||
| else itertools.repeat(transforms.IdentityTransform())) | ||
| for path, trf in itertools.islice(zip(paths, trfs), N): | ||
| yield path, trf + master_transform |
There was a problem hiding this comment.
This is significantly harder to read than what it replaces. Is there a significant performance advantage?
There was a problem hiding this comment.
This was mostly changed for consistency with _iter_collection (where I'd argue that having all the cycling behavior in one place definitely improves readability); here I agree the benefit is less clear and can revert that if you prefer.
There was a problem hiding this comment.
I agree with @jklymak: the original version is more readable (unless you are a functional programming enthusiast, which, without judgement, only few are).
There was a problem hiding this comment.
one vote for the change? I find the new version clearer - for me, the modulo indexing based cycling took a couple of minutes to parse first time I saw it.
There was a problem hiding this comment.
Sure, restored the old version here.
There was a problem hiding this comment.
It's probably fine if you are intimately familiar with itertools, but I definitely am not, whereas I have no problem with modulo.
There was a problem hiding this comment.
itertools is the best, but 🤷♀️ I like the cleanup either way
There was a problem hiding this comment.
I'm not sure it would be measurable here, but I've definitely run into code where itertools was >> "doing math" in terms of runtime performance. I only even mention it since we're in the render loop.
There was a problem hiding this comment.
@dopplershift i can't parse your statement as itertools better or worse
There was a problem hiding this comment.
Generally, itertools better, but not no idea if my runtime argument is meaningful here or not.
This defines all the cycling behavior in a single block, rather than spreading modulo operations all over.
2ce120b to
35c4b07
Compare
This defines all the cycling behavior in a single block, rather than
spreading modulo operations all over.
PR Summary
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).