FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Expand ScalarMappable.set_array to accept array-like inputs by aitikgupta · Pull Request #18870 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Expand ScalarMappable.set_array to accept array-like inputs - #18870

Merged
jklymak merged 6 commits into
matplotlib:masterfrom
aitikgupta:scalarmappable-set_array
May 13, 2021
ConversationCommits (6)ChecksFiles changed
Merged

Expand ScalarMappable.set_array to accept array-like inputs#18870
jklymak merged 6 commits into
matplotlib:masterfrom
aitikgupta:scalarmappable-set_array

Conversation

aitikgupta commented Nov 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

imshow allows set_array to pass lists, Collection does not. (Since _ImageBase overrides set_array of ScalarMappable, adding the ability to pass array-like inputs, and copy the input so changing list after calling function doesn't affect the plots)
This PR expands the ScalarMappable class to make a copy of the original input and casting it to arrays.

Fixes #18841

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • 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).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

aitikgupta commented Nov 2, 2020

Copy link
Copy Markdown
Contributor Author

Note: I didn't pay much attention to the test, as I think it would be better to change the ScalarMappable class itself, instead of overriding it multiple times.
A review is required before following up this approach.

aitikgupta force-pushed the scalarmappable-set_array branch from e7efa10 to 9b926fd Compare November 4, 2020 03:44

aitikgupta commented Nov 4, 2020

Copy link
Copy Markdown
Contributor Author

/cc possible reviewers
@anntzer @story645

QuLogic reviewed Nov 7, 2020
View reviewed changes

QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This seems like something @efiring just looked at?

Comment thread lib/matplotlib/collections.py Outdated

jklymak commented Dec 17, 2020

Copy link
Copy Markdown
Member

as I think it would be better to change the ScalarMappable ...

If that fixes the problem more places, I'd suggest proposing that instead... As it is, this PR is a little scanty on justification.

jklymak added the status: needs comment/discussion needs consensus on next step label Dec 17, 2020

tacaswell commented Dec 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Would it be better to fix this at the ScalarMappable level? 🐑 I should read more carefully before posting.

aitikgupta force-pushed the scalarmappable-set_array branch from 9b926fd to 1e017ed Compare December 18, 2020 03:30

aitikgupta commented Dec 18, 2020

Copy link
Copy Markdown
Contributor Author

I made the changes, but I'm not sure about the location of the test; there's no test_scalarmappable.py in the tests directory.
For now I've added the test in test_collections.py.

aitikgupta changed the title Override set_array for Collection Expand ScalarMappable.set_array to accept array-like inputs Dec 18, 2020
jklymak reviewed Dec 18, 2020
View reviewed changes
Comment thread lib/matplotlib/tests/test_collections.py Outdated

aitikgupta commented Dec 30, 2020

Copy link
Copy Markdown
Contributor Author

I updated the PR summary, hopefully making this a bit easier to understand.

efiring commented Dec 30, 2020

Copy link
Copy Markdown
Member

If #18480 goes in, set_array as modified by this PR will have to be further modified to restore the ability to set_array(None).

jklymak commented Jan 5, 2021

Copy link
Copy Markdown
Member

I'm going to mark as draft until #18480 can be finished but please ping us if that takes too long!

jklymak marked this pull request as draft January 5, 2021 18:00
jklymak removed their request for review January 5, 2021 18:01
aitikgupta force-pushed the scalarmappable-set_array branch 2 times, most recently from a071304 to 9cc32b0 Compare April 29, 2021 01:54
aitikgupta marked this pull request as ready for review April 29, 2021 01:56

aitikgupta commented Apr 29, 2021

Copy link
Copy Markdown
Contributor Author

If #18480 goes in, set_array as modified by this PR will have to be further modified to restore the ability to set_array(None).

Rebased, finished the tests and handled this^

aitikgupta requested review from QuLogic and jklymak April 29, 2021 02:13
timhoffm reviewed Apr 29, 2021
View reviewed changes
Comment thread lib/matplotlib/cm.py Outdated
aitikgupta and others added 2 commits April 29, 2021 14:38
Simplify logic
1139e6a 
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Flake8 fix
2044b57
timhoffm approved these changes Apr 29, 2021
View reviewed changes
Comment thread lib/matplotlib/cm.py Outdated

aitikgupta commented May 6, 2021

Copy link
Copy Markdown
Contributor Author

@jklymak does this still require comment/discussion (label)?

jklymak merged commit a18b4bf into matplotlib:master May 13, 2021
jklymak added this to the v3.5.0 milestone May 13, 2021
QuLogic mentioned this pull request Aug 23, 2021
Merged
3 tasks done
anntzer mentioned this pull request Aug 31, 2021
Closed
anntzer mentioned this pull request Sep 16, 2021
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jklymak jklymak approved these changes
timhoffm timhoffm approved these changes
tacaswell Awaiting requested review from tacaswell
QuLogic Awaiting requested review from QuLogic

Assignees

No one assigned

Labels

topic: collections and mappables

Projects

None yet

Milestone

v3.5.0

Development

Successfully merging this pull request may close these issues.

ScalarMappable should copy its input and allow non-arrays

6 participants

Back | FazBrowse Home | New Git URL