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

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

Merged
merged 6 commits into from May 13, 2021

Conversation

aitikgupta
Copy link
Contributor

@aitikgupta aitikgupta commented Nov 2, 2020

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
Copy link
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
Copy link
Contributor Author

/cc possible reviewers
@anntzer @story645

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

This seems like something @efiring just looked at?

lib/matplotlib/collections.py Outdated Show resolved Hide resolved
@jklymak
Copy link
Member

jklymak commented Dec 17, 2020

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 jklymak added the status: needs comment/discussion needs consensus on next step label Dec 17, 2020
@tacaswell
Copy link
Member

tacaswell commented Dec 17, 2020

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

@aitikgupta
Copy link
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 aitikgupta changed the title Override set_array for Collection Expand ScalarMappable.set_array to accept array-like inputs Dec 18, 2020
@aitikgupta
Copy link
Contributor Author

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

@efiring
Copy link
Member

efiring commented Dec 30, 2020

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
Copy link
Member

jklymak commented Jan 5, 2021

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

@jklymak jklymak marked this pull request as draft January 5, 2021 18:00
@jklymak jklymak removed their request for review January 5, 2021 18:01
@aitikgupta aitikgupta force-pushed the scalarmappable-set_array branch 2 times, most recently from a071304 to 9cc32b0 Compare April 29, 2021 01:54
@aitikgupta aitikgupta marked this pull request as ready for review April 29, 2021 01:56
@aitikgupta
Copy link
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^

lib/matplotlib/cm.py Outdated Show resolved Hide resolved
aitikgupta and others added 2 commits April 29, 2021 14:38
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
lib/matplotlib/cm.py Outdated Show resolved Hide resolved
@aitikgupta
Copy link
Contributor Author

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

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.

ScalarMappable should copy its input and allow non-arrays
6 participants