8000 [WIP] add matrix checking function for quiver input by JunTan · Pull Request #7461 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

[WIP] add matrix checking function for quiver input #7461

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

Closed
wants to merge 5 commits into from
Closed

[WIP] add matrix checking function for quiver input #7461

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d8841a8
add matrix checking function for quiver input
JunTan Nov 15, 2016
bba1ea6
remove the if statement and use isinstance function
JunTan Nov 15, 2016
4bdcac3
change doc string and return type in check_matrix
JunTan Nov 17, 2016
22655b6
move checking into _parse_args
JunTan Nov 22, 2016
73a69a2
refactor _parse_ags
JunTan Nov 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
refactor _parse_ags
  • Loading branch information
@JunTan
JunTan committed Nov 29, 2016
commit 73a69a2d36a947fa76b94f98988fff0d3c5b6a92
13 changes: 6 additions & 7 deletions lib/matplotlib/quiver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,12 +372,13 @@ def _parse_args(*args):
X, Y, U, V, C = [None] * 5
args = list(args)

# The use of atleast_1d allows for handling scalar arguments while also
# keeping masked arrays
if len(args) == 3 or len(args) == 5:
if len(args) == 2 or len(args) == 4:
V = np.atleast_1d(args.pop(-1))
U = np.atleast_1d(args.pop(-1))
elif len(args) == 3 or len(args) == 5:
Copy link
Member

Choose a reason for hiding this comment

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

You are increasing the number of lines of code here, and I don't see any benefit to it. If you left it the way it was, all you would have to do is insert your _check_array into each of the lines extracting U, V, and C, like this:

U = np.atleast_1d(cbook._check_array(args.pop(-1)))

Better yet, you could modify your checking function to use np.atleast_1d instead of np.asanyarray, because the former calls the latter internally. Then in the original you would just replace np.atleast1d with cbook._check_array in each of the 3 lines. Much nicer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that doing the _check_array on U, V, C here (where they are defined) is more suitable than leaving them at the end. But I don't think stuffing the _check_array and atleast_1d together is a good idea, at least, seem less readable to me.

Copy link
Member

Choose a reason for hiding this comment

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

@trpham, I disagree. We need to encapsulate logical chunks of argument validation that typically go together so as to minimize code duplication. The consolidation I am suggesting here is really minimal--it is ensuring that a given argument is some sort of ndarray but not a matrix. That's consistent with the name _check_array (or it could be _ensure_array, etc.) With regard to matrix checking: all of our array-like argument validation should be explicitly blocking (or converting) matrix types because they are known to have odd behavior. This needs to be embedded in the more general validation and "argument scrubbing" functions. We have never claimed to support the matrix subclass, and have always known we didn't want to try to do so--but in the evolution of the library, basic functionality has come first, and ever more stringent validation and scrubbing is gradually being added.

C = np.atleast_1d(args.pop(-1))
V = np.atleast_1d(args.pop(-1))
U = np.atleast_1d(args.pop(-1))
V = np.atleast_1d(args.pop(-1))
U = np.atleast_1d(args.pop(-1))
if U.ndim == 1:
nr, nc = 1, U.shape[0]
else:
Expand All @@ -389,8 +390,6 @@ def _parse_args(*args):
else:
indexgrid = np.meshgrid(np.arange(nc), np.arange(nr))
X, Y = [np.ravel(a) for a in indexgrid]
cbook._check_array(X)
cbook._check_array(Y)
cbook._check_array(U)
cbook._check_array(V)
cbook._check_array(C)
Expand Down
0