BUG: Fix binary_repr for negative numbers #7178
Conversation
|
As noted in my commit message, using Python's built-in
On master : 4.07 usec per loop
On master : 4.19 usec per loop
On master : 2.33 usec per loop
On master : |
|
If it is not possible to represent the number given the supplied width, would it be better to just raise an exception? Silently returning a value with a width different from what was requested seems dangerous. |
|
It's for backwards compatibility reasons. That's what is done with positive numbers, so the behavior should be consistent with negative numbers. |
|
I see. But I still feel it's dangerous, and the positive case could be considered a bug. There are no unit tests for it. Maybe we should take this opportunity to break backward compatibility, and raise an exception in that case? Can anyone more experienced on backward compatibility problems comment? |
|
Well, I would beg to differ in terms of the testing. If you look at the test suite, I explicitly wrote one for insufficient widths to make sure I got the behavior I coded it to do. However, I can understand why you might be "surprised" if you suddenly got a representation with a width larger than you had requested. TBH I don't quite understand why the function was written as it was previously, so another person's input would be great as well! |
|
Oh, I meant there were no existing tests for extending the width past the user's request, meaning it may be unintentional. I do see you added some though :) |
|
Does anyone have anything to add to this? At this point, I'm still leaning towards maintaining backwards compatibility, but other opinions are welcome! |
|
I have to admit I have a slight preference for just erroring out if the width is not enough to represent the twos complement for negative numbers. Unless that is a backwards incompatible break? |
|
@seberg : There would be a backwards compatibility break because that behavior is used for positive numbers, so for consistency, it should be used for negative numbers as well. |
|
Hmmmm, good point. On the other hand, the user suddenly has to know whether his number was positive or negative to make sense of the result. Unrelated, I think we could mention |
|
|
I don't know, but I assumed since it was not used in the function ;). But we are going back to python 1.x for this type of functions I think. Not sure, I guess we would have to deprecate it to throw an error also for positive numbers and maybe just throw an error for negative, since it mostly didn't work anyway. |
|
Uhh, brand new: "New in version 2.6." :) |
|
You mean deprecate behavior when the |
|
Yeah, just because if we would prefer error for negative + insufficient width it seems more right to also do it for positive (and I don't really see a good reason to extend width silently). Still thinking about it, but I think I would like it best. Though this is a bug fix foremost I guess. |
|
Fair enough. I'll make that change then. |
|
@seberg : Added the deprecation, and the tests are passing. Can you merge this? |
|
only after I read it ;p |
|
Touché 😄 |
| binary = bin(twocomp)[2:] | ||
| binwidth = len(binary) | ||
| outwidth = max(binwidth, width) | ||
| warn_if_insufficient(width, binwidth) |
There was a problem hiding this comment.
Can't we just error out already in this branch? I am not sure, will let you pick.
There was a problem hiding this comment.
I feel more comfortable leaving it as a warning to keep the behaviour consistent for both positive and negative numbers. That was one of the problems brought up in the issue was the response was inconsistent between the two sides.
|
Anyway, thanks. Will leave it sitting a bit in case someone disagrees. |
|
Sure thing. I'll ping again in about a week to see if anyone else has anything to add. Would be nice to land this at some point. |
|
I, for one, am satisfied. Merge at will! |
Completely rewrote binary_repr method to use the Python's built-in 'bin' function to generate binary representations quickly. Fixed the behaviour for negative numbers in which insufficient widths resulted in outputs of all zero's for the two's complement. Now, it will return the two's complement with width equal to the minimum number of bits needed to represent the complement. Closes gh-7168.
BUG: Fix binary_repr for negative numbers
|
Thanks @gfyoung! Sorry we've been so slow getting to your PRs... unfortunately we are pretty short on reviewer time right now :-( |
|
@gfyoung: the bottleneck is generlaly someone having the time to read through and think about things; once the decision is made then clicking the button isn't a big deal. The practical consequence is that if the merge conflicts are just a trivial issue with the release notes, then please don't waste your time rebasing over and over... just explain the merge conflict being trivial in the ping so it's clear that the relevant content is still reviewable, and then once you get the 👍 you can rebase once then :-) |
|
@njsmith: Fair point. |
Addresses issue in #7168 in which passing in negative numbers with insufficient
widthparameters resulted in completely incorrect two's complement representations (e.g. all zero's) or even an error. Now, it will return the two's complement with width equal to the minimum number of bits needed to represent the complement.