Skip to content

Fix collection offsets #20717

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 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

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 July 23, 2021 23:10
@anntzer
Copy link
Contributor

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 July 24, 2021 13:04
@QuLogic QuLogic merged commit 9d20dd8 into matplotlib:master Aug 6, 2021
@QuLogic QuLogic removed the status: needs comment/discussion needs consensus on next step label Aug 6, 2021
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Nov 5, 2021
This fixes a regression from matplotlib#20717 in networkx (Fixes matplotlib#21517), but
we'll go forward with the change in a later release to give them time to
fix it.
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Nov 6, 2021
This fixes a regression from matplotlib#20717 in networkx (Fixes matplotlib#21517), but
we'll go forward with the change in a later release to give them time to
fix it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants