Fix hang/incorrect result for ndigits(n,b) when b < 0

[danluu]

This presently hangs for n large enough to trigger the slow division code path in ndigits0z, since we have n::unsigned and we try to put a negative number in there, which turns into a huge unsigned number.

For smaller n, the result is wrong in most cases when b < 0.

This PR doesn't optimize the negative base version the same way the positive base version is optimized, but it at least doesn't hang and works for the cases I've tried.

[IainNZ]

@danluu you are the master at breaking stuff 👍

[JeffBezanson]

Maybe a negative base should just be an error?

[danluu]

It's used occasionally, and handling the negative case correctly should have the same performance impact on the common case as checking for an error, so why not just return the right answer?

[JeffBezanson]

Fair enough.

I notice this is also broken for BigInt.

I think it would be more robust to (1) use integer ceiling division instead of /, and (2) implement this inside ndigits0z, since IIUC your fixed version will only affect calls to ndigits where the first argument is not unsigned.

[danluu]

After changing this, I remembered why I didn't do this inside ndigits0z, which is that ndigits calls unsigned(abs(x)) on n. That works ok for positives bases but that literally always changes the result for when n < 0 && b < 0. Let me fix that and then I'll look at the bigint version.

[JeffBezanson]

Personally I kind of dislike the use of Unsigned in this file; things should be written to work for general integers as much as possible, and unsigned ints just happen to always be non-negative.

[danluu]

I'm just going to throw an error in the bigint case when b < 0 for now, since the right way to fix that seems to be to patch the gmp library. Alternately, I could do the whole calculation outside of the library.

[danluu]

This change would definitely be cleaner if the signed version didn't call the unsigned version. Separating those two seems like an ok thing to do, but what if someone really wants ndigits(::Unsigned, ::Integer)? That can't just call ndigits(signed(n), b) without the possibility of overflowing the conversion.

[JeffBezanson]

I think ideally most functions would use just ::Integer and not ::Signed or ::Unsigned. In theory dispatching on unsigned can shield you from dealing with negativity, but I doubt this is really worth it.

[danluu]

I think some of optimized code will break if things are just changed to ::Integer. For example, ndigits0z has b == 8 && return div((sizeof(n)<<3)-leading_zeros(n)+2,3), but leading_zeros is going to return 0 for a negative number. That works fine now, though, because it's fixed up by calling unsigned(abs(x)) on any signed number.

It should be possible to write something a clean/fast version that doesn't have separate ::Unsigned versions, but that seems like a more invasive change than just fixing the incorrect results we currently have. Would it be ok to merge this and then maybe do a larger refactoring change later?

[JeffBezanson]

Yes a larger refactoring can certainly come later.

[JeffBezanson]

Also, I just merged the cld PR, for this very reason :)

[JeffBezanson]

I don't think this issue is exposed in public APIs, but could cause a problem potentially:

julia> Base.ndigits0z(0, 2)
0

julia> Base.ndigits0z(0, -2)
1

[danluu]

Is that on a previous version? here's what I get on my latest version:

julia> Base.ndigits0z(0, 2)
0

julia> Base.ndigits0z(0, -2)
1

Manually tracing through the code, we have:

ndigits0z(x::Integer, b::Integer) = ndigits0z(unsigned(abs(x)),int(b))
function ndigits0z(n::Unsigned, b::Int)
    ...
    d = 0
    ...
    m = 1
    while m <= n
        m *= b
        d += 1
    end
    return d
end

We should have m = 1, n = 0, which will cause us to skip the loop and just return 0.

[danluu]

Oh, nevermind, I need to change that. I got a stale version when I switched machines.

[JeffBezanson]

Should this call ndigits0znb?

[ivarne]

Is this a bugfix we should @JuliaBackports?

[ivarne]

Answering my own question. No, this is not a minimal bugfix, but depends on other new functionality.

[tkelman]

Looks like this new test was never added to the list in test/runtests.jl. Fixed in 4125f4c