Skip to content

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

Merged
jklymak merged 6 commits intomatplotlib:masterfrom
aitikgupta:scalarmappable-set_array
May 13, 2021
Merged

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

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

aitikgupta commented Nov 2, 2020

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

aitikgupta commented Nov 4, 2020

/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

aitikgupta commented Dec 18, 2020

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
jklymak
jklymak reviewed Dec 18, 2020
View reviewed changes
@aitikgupta
Copy link
Contributor Author

aitikgupta commented Dec 30, 2020

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

aitikgupta commented Apr 29, 2021

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
timhoffm
timhoffm approved these changes Apr 29, 2021
View reviewed changes
@aitikgupta
Copy link
Contributor Author

aitikgupta commented May 6, 2021

@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