Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

aitikgupta
Copy link
Contributor

@aitikguptaaitikgupta commentedNov 2, 2020•
edited
Loading

PR Summary

imshow allowsset_array to pass lists,Collection does not. (Since_ImageBase overridesset_array ofScalarMappable, adding the ability to passarray-like inputs, and copy the input so changing list after calling function doesn't affect the plots)
This PR expands theScalarMappable class to make a copy of the original input and casting it to arrays.

Fixes#18841

PR Checklist

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

@aitikgupta
Copy link
ContributorAuthor

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

@aitikgupta
Copy link
ContributorAuthor

/cc possible reviewers
@anntzer@story645

Copy link
Member

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

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.

aitikgupta reacted with thumbs up emoji

@jklymakjklymak added the status: needs comment/discussionneeds consensus on next step labelDec 17, 2020
@tacaswell
Copy link
Member

tacaswell commentedDec 17, 2020•
edited
Loading

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

@aitikgupta
Copy link
ContributorAuthor

I made the changes, but I'm not sure about the location of the test; there's notest_scalarmappable.py in thetests directory.
For now I've added the test intest_collections.py.

@aitikguptaaitikgupta changed the titleOverride set_array for CollectionExpand ScalarMappable.set_array to accept array-like inputsDec 18, 2020
@aitikgupta
Copy link
ContributorAuthor

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

@efiring
Copy link
Member

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

aitikgupta reacted with thumbs up emoji

@jklymak
Copy link
Member

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

aitikgupta reacted with thumbs up emoji

@jklymakjklymak marked this pull request as draftJanuary 5, 2021 18:00
@jklymakjklymak removed their request for reviewJanuary 5, 2021 18:01
@aitikguptaaitikguptaforce-pushed thescalarmappable-set_array branch 2 times, most recently froma071304 to9cc32b0CompareApril 29, 2021 01:54
@aitikguptaaitikgupta marked this pull request as ready for reviewApril 29, 2021 01:56
@aitikgupta
Copy link
ContributorAuthor

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

Rebased, finished the tests and handled this^

aitikguptaand others added2 commitsApril 29, 2021 14:38
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@aitikgupta
Copy link
ContributorAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jklymakjklymakjklymak approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

@tacaswelltacaswellAwaiting requested review from tacaswell

@QuLogicQuLogicAwaiting requested review from QuLogic

Assignees
No one assigned
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
[8]ページ先頭
©2009-2025 Movatter.jp