Stricter Argument Checking for Flatten Methods by gfyoung · Pull Request #6823 · numpy/numpy

gfyoung · 2015-12-12T04:05:16Z

Addresses bug described in #6598 in which calls to the flatten method using nonsense arguments for the order parameter did not produce errors. This PR changes that behavior so that a TypeError is now thrown unless the order parameter is omitted OR takes on one of the following values (case-sensitive but unicode-insensitive): 'C', 'F', 'A', or 'K'. Here is what the behavior looks like now:

>>> import numpy as np
>>> x = np.zeros((3, 5))
>>>
>>> x.flatten(order=np.pi) # nonsense argument
Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
TypeError: non-string object passed in for the ORDER parameter
>>>
>>> x.flatten(order='CAT') # invalid string argument
Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
TypeError: order not understood

This is my first PR to the numpy library, so all comments regarding the proposed changes as well as the PR itself are welcome!

njsmith · 2015-12-12T04:14:31Z

Mayyybe we should do a deprecation cycle for this, but personally I'd be
fine with waiting to see whether it actually bites anyone in the RC cycle
and only backing off if it does.
On Dec 11, 2015 8:05 PM, "gfyoung" [email protected] wrote:

Addresses bug described in #6598
#6598 in which calls to the flatten
method using nonsense arguments for the order parameter did not produce
errors. This PR changes that behavior so that a TypeError is now thrown
unless the order parameter is omitted OR takes on one of the following
values (case-sensitive but unicode-insensitive): 'C', 'F', 'A', or 'K'.
Here is what the behavior looks like now:

import numpy as np
x = np.zeros((3, 5))
x.flatten(order = np.pi) # nonsense argument
Traceback (most recent call last):
File "", line 1, in
TypeError: non-string object passed in for the ORDER parameter
x.flatten(order = 'CAT') # invalid string argument
Traceback (most recent call last):
File "", line 1, in
TypeError: order not understood

This is my first PR to the numpy library, so all comments regarding the

proposed changes as well as the PR itself are welcome!

You can view, comment on, or merge this pull request online at:

#6823
Commit Summary

BUG: See Issue BUG: flatten method does not validate its argument. #6598

DOC: update documentation for the flatten methods

File Changes

M numpy/add_newdocs.py
https://github.com/numpy/numpy/pull/6823/files#diff-0 (12)

M numpy/core/src/multiarray/conversion_utils.c
https://github.com/numpy/numpy/pull/6823/files#diff-1 (45)

M numpy/matrixlib/defmatrix.py
https://github.com/numpy/numpy/pull/6823/files#diff-2 (10)

Patch Links:

https://github.com/numpy/numpy/pull/6823.patch

https://github.com/numpy/numpy/pull/6823.diff

—
Reply to this email directly or view it on GitHub
#6823.

gfyoung · 2015-12-12T04:21:42Z

Could you clarify what you mean by this (not a lot of experience with working on large open-source projects)? Even if the proposed coding changes are not merged in, I would certainly propose the changes to the documentation, as it seems misleading given the behind-the-scenes functionality

gfyoung · 2015-12-12T04:32:23Z

Yikes. Travis just blew up on me there. Seems like I may have been too strict on the order parameter, or perhaps some other code needs rewriting. Let me take a look.

njsmith · 2015-12-12T07:01:15Z

Sure, to expand:

I think the change is a good one. But, it's possible that it will end up taking code that used to work, and make it start throwing exceptions. Of course that code was probably poorly written if it assumed that it was okay to do np.flatten(arr, order="XYZZY!") or whatever, but these things do sometimes happen, and it's somewhat disproportionate to take code that used to produce correct results and have it suddenly start blowing up without warning because numpy was upgraded.

How much code like this is out there? I don't know. Maybe ~none. Maybe a surprising amount. It's hard to say.

The usual way to handle this kind of issue is with a "deprecation cycle", which goes through several stages: first we change the code so that it still works the same way as before, but it calls warnings.warn("This is gonna break later!", category=DeprecationWarning). Then after some time has passed, we make the actual change we wanted to make in the first place. This has two advantages: (1) every once in a while people actually fix their code before we release the new version, so that users never see it break, and (2) when the code does break, we can say "look, you can't say we didn't warn you" ;-).

This is kinda a borderline case, because arguably anyone who was depending on the broken behavior was depending on a bug, and usually we don't do deprecation warnings for bug fixes. But if this turns out to break lots of code then we still probably want to do the deprecation thing, because making stuff work in this reality is more important that being able to claim that we have virtuously followed all the rules of the alternate reality where everyone does what they're supposed to...

So what I was saying was that: this is a borderline case where it isn't 100% obvious whether we need a deprecation cycle or not -- this is the sort of thing that has to be figured out when reviewing a change -- but I'd be fine with merging it, so long as we keep an eye out for it causing problems when we put out a Release Candidate for people to test, and possibly revert it then and take the slower route.

gfyoung · 2015-12-12T07:11:27Z

Okay, I understand. I have been starting to see what you mean already, as my subsequent commits have been catching errors in which booleans were passed in the order parameter, causing tests to break on Travis.

njsmith · 2015-12-12T07:25:59Z

Oh yeah, that is a bad sign... if order=True and order=False used to be synonyms for "F" and "C" respectively (whyyyy??) then we might well be stuck doing a deprecation cycle on those at least.

njsmith · 2015-12-12T07:27:55Z

A quick skim of scipy source finds lots of lowercase "c" and "f", but no bools, at least. (Though I might have missed them.)

gfyoung · 2015-12-12T07:50:05Z

Oh, that's right. I forgot that scipy code might also be broken if there are any major changes to numpy. Depending on how the builds are setup in scipy, I'm not entirely sure how the tests would indicate that changes in numpy broke scipy for example. However, I seem to have caught all of the immediate issues just in the numpy library, as Travis is finally happy with my changes.

njsmith · 2015-12-12T07:55:09Z

It's not just scipy -- there are literally hundreds of libraries out there that use numpy, and any of them can (potentially) be broken by major changes in numpy.

scipy is a convenient one to check though, because it's especially widely used and because it's even older then numpy so has a special habit of using grotty old conventions from the bad old days :-)

gfyoung · 2015-12-12T08:09:11Z

Fair enough. Do you think it's best then that I role back some of the changes I made to the C code and replace them with PyErr_WarnEx(PyExc_DeprecationWarning, "BLAH BLAH BLAH", 1) or something of that sort?

seberg · 2015-12-12T12:34:39Z

Yes, we have a DEPRECATE macro to make it a bit shorter. And we need a test for it. There is a test_deprecations test file where it could go. Best, add a short date and numpy version comment to it (numpy version will be 1.11, just something like /* 2014-06-12, 1.9 */ before the DEPRECATE macro).

jaimefrio · 2015-12-12T13:58:28Z

Do we really want to make lowercase versions an error? It seems a little too strict to me...

gfyoung · 2015-12-12T19:37:09Z

@seberg : I'm not entirely sure I follow what you were saying. Could you explain?

@jaimefrio : I guess it's an issue between what you are allowed to do and what the documentation says you can do. If I'm not mistaken, I don't think the documentation says you can use lowercase. Perhaps the documentation should then be expanded to state this?

jaimefrio · 2015-12-12T20:35:18Z

Well, Nathaniel did mention there were tests in scipy using lowercase that would be broken by this change...

gfyoung · 2015-12-12T20:47:17Z

Right, but I guess the question is: should we allow people to pass in lowercase parameters? That will determine what sort of warning I could put in the C code instead of the changes I made.

seberg · 2015-12-13T08:43:10Z

@gfyoung, I mean that yes, please change the pull request to give deprecations. Just wanted to note where tests would go and DEPRECATE macro we have.
I agree with Jaime though, I don't think forbidding lower case will improve much at all, the meaning seems obvious in any case so it seems it would not achieve much except bugger people to change code that will seem fine to them.

gfyoung · 2015-12-13T12:12:40Z

Fair enough. I updated the PR to use the DEPRECATE macro instead. Is the implementation correct?

seberg · 2015-12-14T08:55:38Z

DEPRECATE needs to be

if (DEPRECATE("blabla") < 0) { cleanup_code/goto; return; }

since it raises an error when the warnings are set accordingly. Also, please add a short test for both deprecations into the test_deprecations.py file, that would also find such an issue.

I'm sorry: could you clarify what you mean by this? I tried reading the documentation on Python Docs, but I'm not sure I quite follow still.

Deprecate/PyErr_WarnEx can return an error value (specifically, this happens if you do for example import warnings; warnings.simplefilter("error", DeprecationWarning).
In that case you need to check for a negative return, do the typical cleanup (which might be nothing in this case) and return an error return (NULL or -1 normally).

If you are curious: https://docs.python.org/2/c-api/exceptions.html#c.PyErr_WarnEx the second paragraph mentions it, the error handling code you can probably steal/infer from other error handling in the funciton.

gfyoung · 2015-12-14T11:05:43Z

@seberg : Ah, I think I understand. PR updated with a still happy Travis.

seberg · 2015-12-14T12:32:27Z

Yeah, this is right. Can you add a comment /* 2015-12-14, 1.11 */ above both? I think the \ might be unnecessary here, since it is all in brackets? Or was that different in C, don't remember?

Other nitpick, I think I prefer order in lower case maybe with backticks, but frankly I doubt we have a consistent style, so not unless someone else complains soon. I am not sure we should name ORDER explicitly, since in principle the argument could be called differently. Don't have a much nicer idea though, maybe someone else has ;).

Maybe also just: "invalid order; only 'C', 'F', 'A', or 'K' can be converted to a memory/iteration order." (could add what was actually given, but it tends to be a bit annoying). Anyway, as I said, I am not good at writing these, just thinking loud :).

You only need \ in multiline defines.

@seberg : Regarding the comments, sure thing

Good point on the \. It's hard juggling multiple languages at once. :) I did a slight revision to the warning messages to not explicitly mention the order parameter

seberg · 2015-12-14T12:36:40Z

Anyway, thanks, almost there, will leave a bit in case someone has some nitpicks about the message or so.

seberg · 2015-12-14T12:38:40Z

Oh, sorry :(, one more thing, can wait a bit, but maybe you can add a short note to the release notes in doc/release/ for 1.11, we have most deprecations in there, and if we don't add it now, we would just forget about it later.

jaimefrio · 2015-12-14T19:12:09Z

PEP8 prefers putting the whole string inside parenthesis, and getting rid of both the \s and the +s, relying on string literal concatenation.

Good to know. Will do.

gfyoung · 2015-12-14T23:24:03Z

@seberg : No worries. I perfectly understand wanting to make sure the PR is just right. It's difficult to undo a bad merge. :) - I updated the release notes with the deprecation.

seberg · 2015-12-15T09:52:04Z

It is more general, this is used (almost) everywhere where an order argument is used. Also, I think you don't have to argue that it should not be allowed, it will not be allowed in the future ;). Anyway, the document will be revised again for the release.

When you are done with this, could you squash the commits together into one or two (rebase -i HEAD~7 or similar for example). Best give it a commit like "DEP: ..." and you can include "closes gh-6598" at the end in the message as well (the message will be closed automatically and we have the reference).

Oh, and don't worry about the test failure, seems there is something else going on.

Regarding the release note, sure thing. Regarding the test failure, got it. I think several people (myself included) have brought this up in the discussion mailing list.

charris · 2015-12-16T18:56:02Z

Close and reopen to restart tests.

gfyoung · 2015-12-16T20:24:32Z

@charris : Thanks! If Travis is happy, and if everyone else is happy, this should be good to merge.

seberg · 2015-12-18T10:33:29Z

Sorry for the delay. If you got some patience left, the best form would be to have:

DEP: short description

longer description

closes gh-#

The DEPRECATE strings could be aligned more PEP8 like and the closing bracket on the same line ;). But I think I will merge this in a bit in any case. Lets hope it only annoys a moderate number of people ^^.

gfyoung · 2015-12-18T10:50:33Z

No worries. #6824 has been keeping me occupied :) This PR is good to merge I feel, so tidying it up a bit doesn't hurt at all. Will make those adjustments and make sure Travis is still happy.

The bug traces to the PyArray_OrderConverter method in conversion_utils.c, where no errors are thrown if the ORDER parameter passed in is not of the string data-type or has a string value of length greater than one. This commit causes a DeprecationWarning to be raised, which will later be turned into a TypeError or another type of error in a future release. Closes gh-6598.

gfyoung · 2015-12-18T11:45:20Z

32-bit build was hanging for no apparent reason. Closing and reopening PR to see if that was just an anomaly.

gfyoung · 2015-12-18T12:15:23Z

@seberg : Travis is happy again! Style suggestions implemented successfully. Should be ready to merge.

seberg · 2015-12-18T12:20:52Z

OK, lets give it a shot. Thanks a lot!

Stricter Argument Checking for Flatten Methods

charris added component: numpy._core component: numpy.lib component: numpy.matrixlib 01 - Enhancement labels Dec 12, 2015

seberg reviewed Dec 14, 2015
View reviewed changes

jaimefrio reviewed Dec 14, 2015
View reviewed changes

seberg reviewed Dec 15, 2015
View reviewed changes

charris closed this Dec 16, 2015

charris reopened this Dec 16, 2015

gfyoung closed this Dec 18, 2015

gfyoung reopened this Dec 18, 2015

seberg added a commit that referenced this pull request Dec 18, 2015

Merge pull request #6823 from gfyoung/order_arg_validate

3af5f05

Stricter Argument Checking for Flatten Methods

seberg merged commit 3af5f05 into numpy:master Dec 18, 2015

gfyoung deleted the order_arg_validate branch December 18, 2015 12:22

sethtroisi mentioned this pull request Sep 26, 2019

DEP: Complete deprecation of invalid array/memory order #14596

Merged

Uh oh!

Uh oh!

Conversation

gfyoung commented Dec 12, 2015

Uh oh!

njsmith commented Dec 12, 2015

proposed changes as well as the PR itself are welcome!

Uh oh!

gfyoung commented Dec 12, 2015

Uh oh!

gfyoung commented Dec 12, 2015

Uh oh!

njsmith commented Dec 12, 2015

Uh oh!

gfyoung commented Dec 12, 2015

Uh oh!

njsmith commented Dec 12, 2015

Uh oh!

njsmith commented Dec 12, 2015

Uh oh!

gfyoung commented Dec 12, 2015

Uh oh!

njsmith commented Dec 12, 2015

Uh oh!

gfyoung commented Dec 12, 2015

Uh oh!

seberg commented Dec 12, 2015

Uh oh!

jaimefrio commented Dec 12, 2015

Uh oh!

gfyoung commented Dec 12, 2015

Uh oh!

jaimefrio commented Dec 12, 2015

Uh oh!

gfyoung commented Dec 12, 2015

Uh oh!

seberg commented Dec 13, 2015

Uh oh!

gfyoung commented Dec 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfyoung commented Dec 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented Dec 14, 2015

Uh oh!

seberg commented Dec 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfyoung commented Dec 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Dec 16, 2015

Uh oh!

gfyoung commented Dec 16, 2015

Uh oh!

seberg commented Dec 18, 2015

Uh oh!