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

Quadmesh.set_array validates dimensions#20215

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
jklymak merged 1 commit intomatplotlib:masterfromjeffreypaul15:mpl-17508
Jun 10, 2021

Conversation

jeffreypaul15
Copy link
Contributor

@jeffreypaul15jeffreypaul15 commentedMay 12, 2021•
edited
Loading

PR Summary

PR Checklist

Fixes#17508

  • 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).

Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@jklymak
Copy link
Member

This needs to pass the "checks" above, whereas it seems a lot is broken ;-)

jeffreypaul15 reacted with thumbs up emoji

@jklymakjklymak marked this pull request as draftMay 12, 2021 20:15
@jklymak
Copy link
Member

Drill down on those tests to see what fails, and then I usually work on them on my home machine until they pass, but that does mean you need to setup py.test locally.

@jeffreypaul15
Copy link
ContributorAuthor

jeffreypaul15 commentedMay 12, 2021•
edited
Loading

Drill down on those tests to see what fails, and then I usually work on them on my home machine until they pass, but that does mean you need to setup py.test locally.

I'm a little confused on how the coverage is decreasing, all of the new code is tested and works fine but the coverage seems to be an issue.

@jeffreypaul15
Copy link
ContributorAuthor

@jklymak Does this need a mention in the api_changes?

@jklymak
Copy link
Member

I'm not sure - did it raise a type error before? If the error type is different that probably merits a API change note

@jklymakjklymak marked this pull request as ready for reviewMay 13, 2021 14:03
@jeffreypaul15
Copy link
ContributorAuthor

jeffreypaul15 commentedMay 13, 2021•
edited
Loading

I'm not sure - did it raise a type error before? If the error type is different that probably merits a API change note

The changes raise an error when faulty data is passed toset_array so I'm assuming this requires a note.

@jklymak
Copy link
Member

Sure, but presumably faulty data raised an error before, at least at draw time?

@jeffreypaul15
Copy link
ContributorAuthor

jeffreypaul15 commentedMay 13, 2021•
edited
Loading

Sure, but presumably faulty data raised an error before, at least at draw time?

At draw time yes, but not while usingset_array though, so the user could pass array of any shape toset_array.

if isinstance(self, mpl.collections.QuadMesh):
A_ = A.ravel()
width, height = self._meshWidth, self._meshHeight
if self._shading in ['flat', 'nearest', 'auto']:
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have seen this sooner. This check isn't compatible withflat orauto - please see the docstring for pcolormesh...

Copy link
Member

Choose a reason for hiding this comment

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

I think this function needs to be called when the QuadMesh is initialized to make sure it is compatible with the initialization. I don't know if that means we want to drop extra data forflat or not.

Copy link
ContributorAuthor

@jeffreypaul15jeffreypaul15May 13, 2021•
edited
Loading

Choose a reason for hiding this comment

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

Ah sorry I misunderstood howauto works.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I think this function needs to be called when the QuadMesh is initialized to make sure it is compatible with the initialization. I don't know if that means we want to drop extra data forflat or not.

When quadmesh is initialized the arguments are already validated for faulty data right?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Also, when shading =flat, shouldset_array only accept data of same shape as X and Y (where extra data is dropped) or should it accept data of where it's dimensions are one less than X and Y, or should it accept both?

Copy link
Member

Choose a reason for hiding this comment

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

So the shading for A [NxM]

nearest: x[M], y[N]
flat: x [M+1], y[N+1] is preferred, but for back-combat x[M], y[N] works by dropping last row and column of A (so it is now [N-1 x M-1]
auto: Choosesnearest if x [M], y[N], orflat if x [M+1], y [N+1]
gouraud: x[M+1], y[N+1].

Copy link
ContributorAuthor

@jeffreypaul15jeffreypaul15May 13, 2021•
edited
Loading

Choose a reason for hiding this comment

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

Ah, okay thanks for explaining it.
One more small question I've got is


When shading is set toauto and like you've mentionedauto: Choosesnearest if x [M], y[N], orflat if x [M+1], y [N+1].
Shading is set back to being flat when x[M[, y[N] and C is of the same shape, this basically causesQuadmesh._shading to be set toflat regardless of theshading set toauto, it's nevernearest when set toauto.

@jeffreypaul15
Copy link
ContributorAuthor

@jklymak I've made these changes and it should account for when shading isflat, let me know if this is fine

A_ = A.ravel()
width, height = self._meshWidth, self._meshHeight
if self._shading == 'flat':
if not (len(A_) == width * height or
Copy link
Member

Choose a reason for hiding this comment

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

I think we now need to check all these, not only that they error, but that they work if the set_array is correct? Like I'm still not sure what happens if you have shading=flat and pass len(A) == width*height after the fact.

Sorry, this might not have really been a good fist issue because of the goofy semantics of this.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, no worries :) before any of the changes made, I had a look at this, when len(A) == (width+1)(height+1) andshading =flat, the same thing happens where the last dimensions are dropped. When len(A) == widthheight, then it's the same thing as setting A with the dimensions of one less than X and Y.
Here, when shading is set toflat, regardless of the shape ofC passed topcolormesh, the height (self._meshHeight) and width (self._meshWidth) are always the same shape as one less than X and Y.
For example :

# shape of xe is (10,)# shape of ye is (14,)# shape of c is (14,10)col=plt.pcolormesh(xe,ye,c,shading='flat')# works with drops the dimensions to get (13, 9) accordingly.# But self.+_meshHeight, self._meshWidth is = (13, 9)# when# shape of xe is (10,)# shape of ye is (14,)# shape of c is (13,9)col=plt.pcolormesh(xe,ye,c,shading='flat')# works as intended without dropping any dimensions.# And self.+_meshHeight, self._meshWidth is = (13, 9), same as that when c is of (14, 9)

So my point is regardless of the shape of A passed toset_array, in the example listed above it should either be (13, 9) or (14, 10) as self._meshHeight and self._meshWidth is already automatically set to (13, 9) if the dimensions are dropped

Copy link
Member

Choose a reason for hiding this comment

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

OK, just checking. I'd forgotten if the dropped row/column happened when it was ingested or when it was drawn...

jeffreypaul15 reacted with thumbs up emoji
A_ = A.ravel()
width, height = self._meshWidth, self._meshHeight
if self._shading == 'flat':
if not (len(A_) == width * height or
Copy link
Member

Choose a reason for hiding this comment

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

Um, don't you have to check width and height separately? Otherwise, the dfiffernce between (3, 4) and (4, 3) would not be catched.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, yikes I forgot about that case. My apologies, I'll fix this.

Copy link
ContributorAuthor

@jeffreypaul15jeffreypaul15May 14, 2021•
edited
Loading

Choose a reason for hiding this comment

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

The simplest way seemed to be doing it with a flag, otherwise we'd have to repeat

raiseTypeError(f"Dimensions of A{shape} are incompatible with X ({width}) and/or Y ({height})")

multiple times

width, height = self._meshWidth, self._meshHeight

if self._shading == 'flat':
if len(A.shape) == 1:
Copy link
Member

Choose a reason for hiding this comment

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

Do we allowlen(A.shape())==1 arrays to be passed and they get reshaped somehow? If so, I'm not sure why we would block other arrays that fit? I tried to track down what happens but could not quickly...

Copy link
ContributorAuthor

@jeffreypaul15jeffreypaul15May 14, 2021•
edited
Loading

Choose a reason for hiding this comment

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

I was under the assumption that we only allow flattened arrays that fit, isn't that true? I'm not sure I understand what other arrays that fit mean

Copy link
Member

Choose a reason for hiding this comment

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

I was under the assumption that we only allow flattened arrays that fit, isn't that true?

I don't know. That is what I am asking.

@jeffreypaul15
Copy link
ContributorAuthor

jeffreypaul15 commentedMay 14, 2021•
edited
Loading

Actually, before any of the changes were made,pcolormesh callsset_array under the hood and it passesC.ravel() to it. From what I've checked, passing an arrayA ofheight*width orwidth*height would still produce the same plot.

So based on@timhoffm's suggestion, I don't really think we should be checking for height and width separately.
Whenlen(A.shape())==1 it should just match the X and Y dimensions (based on shading) when flattened.

@jklymak
Copy link
Member

From what I've checked, passing an array A of heightwidth or widthheight would still produce the same plot.

Right, so the new checks you've added would be an API change, which I don't think we want to get into here. So maybe you should go back to what you had before where you just checked that len(A.ravel()) == height*width (etc). Unless@timhoffm wanted an API change (which wouldn't be terrible, but we would need to make sure the API change note reflected that)

@jeffreypaul15
Copy link
ContributorAuthor

From what I've checked, passing an array A of height_width or width_height would still produce the same plot.

Right, so the new checks you've added would be an API change, which I don't think we want to get into here. So maybe you should go back to what you had before where you just checked that len(A.ravel()) == height*width (etc). Unless@timhoffm wanted an API change (which wouldn't be terrible, but we would need to make sure the API change note reflected that)

Yeah, makes sense. I'll revert back to those changes. Thanks for explaining it :)

@jklymak
Copy link
Member

I'd wait for Tim to comment. He is the API boss but he may not have realized this is a change.

@jeffreypaul15
Copy link
ContributorAuthor

I'd wait for Tim to comment. He is the API boss but he may not have realized this is a change.

Yes, will do

@timhoffm
Copy link
Member

timhoffm commentedMay 15, 2021•
edited
Loading

In the light of#18472 (comment) and my reply below that I propose the following:

  • For all inputs: raise if the total number of elements does not match.
  • For 2D inputs:_api.warn_deprecated() if the shape does not match.
  • For nD (N>=3) inputs_api.warn_deprecated()

While color values are flattened before passing them to the renderer, it does not make sense to accept other shapes of matching total number of elements or even higher-dimensional data.

Whether we should deprecate 1D inputs as well should be discussed separately. This would violate the Liskov substitution principle. We'd have to weigh that against a cleaner more narrow API.

jeffreypaul15 reacted with thumbs up emoji

@timhoffm
Copy link
Member

@jeffreypaul15 I'm sorry, I currently don't have bandwidth to look into this. Will take a couple of days for a response.

@jeffreypaul15
Copy link
ContributorAuthor

@jeffreypaul15 I'm sorry, I currently don't have bandwidth to look into this. Will take a couple of days for a response.

No worries

@QuLogic
Copy link
Member

I think your other PRs are in? You should rebase to remove those changes from this one.

jeffreypaul15 reacted with thumbs up emoji

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Almost good to go. Only some comment/documentation to do.

@@ -2047,6 +2046,40 @@ def set_paths(self):
self._paths = self._convert_mesh_to_paths(self._coordinates)
self.stale = True

def set_array(self, A):
Copy link
Member

Choose a reason for hiding this comment

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

This is public API, so it needs a docstring.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, will do.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I'm not really sure how to structure this docstring, please make any changes if required

Copy link
Member

Choose a reason for hiding this comment

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

Well, it would have inherited a docstring fromScalarMappable, I think.

"""
Set the value array from array-like *A*.
Parameters
----------
A : array-like or None
The values that are mapped to colors.
The base class `.ScalarMappable` does not make any assumptions on
the dimensionality and shape of the value array *A*.
"""

Though some of that needs to be overridden anyway, so it's good to write one here.

jeffreypaul15 reacted with thumbs up emoji
Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Sorry, my docstring suggestions apparently had unneeded spaces 🙄 , which cause flake8 to complain.

You can directly merge the corrections from the github interface. We'll squash-merge this PR anyway.

@@ -2047,6 +2046,40 @@ def set_paths(self):
self._paths = self._convert_mesh_to_paths(self._coordinates)
self.stale = True

def set_array(self, A):
Copy link
Member

Choose a reason for hiding this comment

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

Well, it would have inherited a docstring fromScalarMappable, I think.

"""
Set the value array from array-like *A*.
Parameters
----------
A : array-like or None
The values that are mapped to colors.
The base class `.ScalarMappable` does not make any assumptions on
the dimensionality and shape of the value array *A*.
"""

Though some of that needs to be overridden anyway, so it's good to write one here.

jeffreypaul15 reacted with thumbs up emoji
@timhoffmtimhoffm dismissedjklymak’sstale reviewJune 8, 2021 18:55

The requested change has been addressed.

@jklymak
Copy link
Member

This is great@jeffreypaul15 Did you want to rebase to just one commit? We can squash it from our end if you prefer...

@jklymakjklymak marked this pull request as draftJune 10, 2021 14:14
@jklymak
Copy link
Member

https://www.internalpointers.com/post/squash-commits-into-one-git is as good a guide as any to how to do this...

@jeffreypaul15
Copy link
ContributorAuthor

https://www.internalpointers.com/post/squash-commits-into-one-git is as good a guide as any to how to do this...

Thanks, but I've got a history of messing things up when squashing. I'll have a look at this and in the meantime you can squash it, thanks for reviewing all of this repeatedly 😄

@jklymak
Copy link
Member

.. I pinged you on gitter

@jeffreypaul15jeffreypaul15force-pushed thempl-17508 branch 5 times, most recently fromb9bfdb3 tof4d2d61CompareJune 10, 2021 15:11
@jeffreypaul15jeffreypaul15 marked this pull request as ready for reviewJune 10, 2021 15:13
@jklymakjklymak merged commitee577c5 intomatplotlib:masterJun 10, 2021
@jklymak
Copy link
Member

Congratulation@jeffreypaul15 Sorry that was such a tough one!

@jeffreypaul15
Copy link
ContributorAuthor

Congratulation@jeffreypaul15 Sorry that was such a tough one!

Haha glad that it's done. Thanks for the help and multiple reviews

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

@QuLogicQuLogicQuLogic left review comments

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@jklymakjklymakjklymak approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

Quadmesh.set_array should validate dimensions
4 participants
@jeffreypaul15@jklymak@timhoffm@QuLogic
[8]ページ先頭
©2009-2025 Movatter.jp