DEP: Deprecated using a float index in linspace #7328
Conversation
| except TypeError: | ||
| i = int(i) | ||
| msg = "Indexing with a float is deprecated "\ | ||
| "and will be removed in the future." |
There was a problem hiding this comment.
Line this up with the opening quote on the previous line.
There was a problem hiding this comment.
Also, use parentheses for line continuation instead of backslashes
|
This is looking pretty good to me -- the one other thing is that we have some conventions for keeping track of deprecations so that we remember to follow up on them, and I'm not totally sure what we've been doing recently :-). You should at least add a comment like |
|
Should there also probably be something in the release notes? |
|
Frankly, not sure, Chuck probably has something he likes, I used to have follow up issues. But either Chuck did not like them, or they all outlived their deprecations, I am not sure :). I think I would prefer to remove the word "indexing", the python error says "cannot be interpreted as an index". While "index" is more correct in the sense it includes that it is limited by ssize_t, I think "safely interpreted as an integer" or something saying integer is even clearer. "indexing" sounds too much like the square bracket notation this has nothing to do with. |
|
Updated. We usually put deprecation tests in |
| try: | ||
| i = operator.index(i) | ||
| except TypeError: | ||
| msg = "%f cannot be safely interpreted as an integer." % i |
There was a problem hiding this comment.
Thanks, maybe rather "object of type {} ...".format(type(i)) or just not %f but %s/{}. The object might also be a string in principle, and things would crash rather cryptically then. I admit it is very very unlikely, but since we may want to use this function elsewhere.
There was a problem hiding this comment.
Agreed with @seberg -- we still have a responsibility to provide sensible error messages for basic mistakes like np.linspace(0, 1, np.float32) (oops, user forgot to write dtype=).
|
@emilienkofman Be good to get this finished up. |
|
Sorry for the conflicts. I don't know exactly what happened. I tried to fit the test in |
|
Conflicts happen when other PRs are merged, nothing that is your fault. You can start by squashing your commits, |
|
Chuck's suggesting you do things in two stages: first "squash" your commits down into one commit, which will involve rebasing them onto the same revision they're already based on, like in the output you pasted. Then, once you've successfully done that, as a second step rebase that one commit onto the most recent commit on master branch. The advantage of doing things this way is that if you try to rebase onto master immediately, then you have to be careful or you might find yourself resolving the same conflicts repeatedly for each of your commits, but if you have just one commit then the worst case is that you just have to resolve the conflicts once :-) |
|
Ok I will eventually get it right at some point, I did the first rebase by squashing all my commits into one which I think worked (I can see it into gitk for instance). Then Edit: I did: Edit: Finally got it I think. Sorry that's a bit laborious. |
DEP: Deprecated using a float index in linspace DEP: Deprecated using a float index in linspace DEP: Deprecated using a float index in linspace DEP: Deprecated using a float index in linspace DEP: Deprecated using a float index in linspace DEP: Release notes for PR#7328
|
Looks good to me. Congratulations on winning against git :-). Tests haven't finished + I'm on mobile + @charris was looking at this most recently (for 1.11 I think) and I don't want to step on him accidentally, so will leave the merge to him (or whoever). |
| @@ -148,5 +148,8 @@ Deprecations | |||
|
|
|||
| Assignment of ndarray object's ``data`` attribute | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |||
| Assigning the 'data' attribute is an inherently unsafe operation as pointed out | |||
| in gh-7083. Such a capability will be removed in the future. | |||
| Assigning the 'data' attribute is an inherently unsafe operation as pointed out in gh-7083. Such a capability will be removed in the future. | |||
There was a problem hiding this comment.
Keep lines < 80 characters long, this one is 142 ;) I'll fix it up later, but keep that in mind.
There was a problem hiding this comment.
Might as well do it, as other fixes need to be made.
|
Do you think this could be merged or do you suggest that we investigate more on the |
|
Should be good, unless Chuck still has doubts. Will merge in a bit unless anyone has complains or beats me to it. |
|
I don't have any substantive objections. |
DEP: Deprecated using a float index in linspace
|
Thanks @emilienkofman . |
|
Thanks Chuck and everyone. |
According to what was discussed in Issue #7289.