Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 13, 2021
Merged

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

merged 6 commits into from
May 13, 2021

Conversation

aitikgupta
Copy link
Contributor

@aitikgupta aitikgupta commented Nov 2, 2020 •
edited
Loading

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 aitikgupta force-pushed the scalarmappable-set_array branch from e7efa10 to 9b926fd Compare November 4, 2020 03:44
@aitikgupta
Copy link
Contributor Author

/cc possible reviewers
@anntzer @story645

QuLogic
QuLogic reviewed Nov 7, 2020
View reviewed changes
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?

@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 •
edited
Loading

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

@aitikgupta aitikgupta force-pushed the scalarmappable-set_array branch from 9b926fd to 1e017ed Compare December 18, 2020 03:30
@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^

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

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

@jklymak jklymak merged commit a18b4bf into matplotlib:master May 13, 2021
@jklymak jklymak added this to the v3.5.0 milestone May 13, 2021
@QuLogic QuLogic mentioned this pull request Aug 23, 2021
Merged
3 tasks
@anntzer anntzer mentioned this pull request Aug 31, 2021
Closed
@anntzer anntzer mentioned this pull request Sep 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

@tacaswell tacaswell Awaiting requested review from tacaswell

@QuLogic 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
@aitikgupta @jklymak @tacaswell @efiring @QuLogic @timhoffm