Skip to content
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

Fix collection offsets #20717

Merged
merged 5 commits into from Aug 6, 2021
Merged

Conversation

@fourpoints
Copy link
Contributor

@fourpoints fourpoints commented Jul 22, 2021

PR Summary

Made some suggested changes related to issue #20698.

  • Merged Collection._uniform_offsets and Collection._offsets, removed the former, kept the latter. Any function dependent on this variable was removed.
  • Removed Collection._offsetsNone, replaced with a check if offsets were non-zero where used.
  • Added Collection.set_offset_transform().

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [N/A] New features are documented, with examples if plot related.
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

I did add Collection.set_offset_transform(), but this isn't really a new feature, since it can already be set during initialization, so I'm not sure if/how it should be documented. I can add a mention if necessary.


Update 2021-07-23 09:47:

  • Copied the body of Artist.set_transform() to Collection.set_offset_transform() to be consistent.
  • Added a release note.
@tacaswell tacaswell added this to the v3.5.0 milestone Jul 23, 2021
@tacaswell
Copy link
Member

@tacaswell tacaswell commented Jul 23, 2021

This should be signed off on by @anntzer as I want to make sure this is not putting back something we removed recently.

@tacaswell tacaswell requested a review from anntzer Jul 23, 2021
@anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 24, 2021

@tacaswell If you are specifically worried about #13696: no, this PR is not doing anything like that.
This is indeed fixing some weirdness (unrelated to #13696) I had noticed some time ago, but I haven't fully unraveled the old codepaths so I don't have any opinion about the correctness of the new version (it may well be correct, I just don't know :-))

@anntzer anntzer removed their request for review Jul 24, 2021
lib/matplotlib/collections.py Outdated Show resolved Hide resolved
lib/matplotlib/collections.py Show resolved Hide resolved
doc/users/next_whats_new/collection_offsets.rst Outdated Show resolved Hide resolved
lib/matplotlib/collections.py Show resolved Hide resolved
@QuLogic
QuLogic approved these changes Aug 6, 2021
@QuLogic QuLogic merged commit 9d20dd8 into matplotlib:master Aug 6, 2021
25 checks passed
25 checks passed
@github-actions
flake8
Details
@github-actions
greeting
Details
@github-actions
Python 3.7 on ubuntu-18.04 (Minimum Versions)
Details
@github-actions
Python 3.7 on ubuntu-18.04
Details
@github-actions
Python 3.8 on ubuntu-18.04
Details
@github-actions
Python 3.9 on ubuntu-20.04
Details
@github-actions
Python 3.8 on macos-latest
Details
@github-actions
eslint
Details
@lgtm-com
LGTM analysis: JavaScript No code changes detected
Details
@lgtm-com
LGTM analysis: Python 1 fixed alert
Details
@codecov
codecov/patch 93.75% of diff hit (target 50.00%)
Details
@codecov
codecov/project/library 80.50% (target 50.00%)
Details
@codecov
codecov/project/tests 98.98% (+0.00%) compared to 57db5a7
Details
@appveyor
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@azure-pipelines
matplotlib.matplotlib Build #20210801.13 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Check Skip) Check Skip succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Linux_py37) Main Pytest Linux_py37 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Linux_py38) Main Pytest Linux_py38 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Linux_py39) Main Pytest Linux_py39 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Windows_py37) Main Pytest Windows_py37 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Windows_py38) Main Pytest Windows_py38 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest Windows_py39) Main Pytest Windows_py39 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest macOS_py37) Main Pytest macOS_py37 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest macOS_py38) Main Pytest macOS_py38 succeeded
Details
@azure-pipelines
matplotlib.matplotlib (Main Pytest macOS_py39) Main Pytest macOS_py39 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants