x % Unsigned, x % Signed #34864

JeffreySarnoff · 2020-02-25T00:15:52Z

This follows from a discussion on Slack (exerpted below); the implementation is from @Keno.

Matt Bauman: I guess I’m in a gripey mood today. Is there a reason why we don’t allow -1 % Unsigned?

julia> -1 % UInt
0xffffffffffffffff
julia> -1 % Unsigned
ERROR: InexactError: check_top_bit(Int64, -1) ...

Matt Bauman: Is the “correct” way to do this unsigned(-1)?

Keno Fischer: yes, but I think % Unsigned is reasonable.
...

Kristoffer Carlsson: Why [is reinterpret] yuk? Isn't that the most descriptive function name to use? Who knows what semantics unsigned has?

Matt Bauman: For some reason, reinterpret feels like it imposes a higher cognitive burden — like I need to think about the exact bit representation. All I care about in this operation is ensuring that negative values become absurdly large. But it’s a moot point.

Keno Fischer: In this case rem is just using the convert fallback. I think a definition like

rem(x::Signed, ::Type{Unsigned}) = x % unsigned(typeof(x))

would be reasonable ... if somebody wants to submit a PR ... https://github.com/JuliaLang/julia/blob/master/base/int.jl#L508-L510

Stefan Karpinski: @KristofferC: the reason I don’t like reinterpret is that it forces the type of the input and the result to have the same size but this is a perfectly well-defined operation even for different sizes.

@Keno

This follows from a discussion on Slack; the implementation was suggested by @Keno.

Keno · 2020-02-25T00:17:01Z

base/int.jl

@@ -506,6 +506,7 @@ if nameof(@__MODULE__) === :Base
 end

 rem(x::T, ::Type{T}) where {T<:Integer} = x
+rem(x::Signed, ::Type{Unsigned}) = x % unsigned(typeof(x)) 


Should probably also have the opposite x % Signed.

done and tests are also done

JeffreySarnoff · 2020-02-25T00:47:58Z

Covered News.md, src/int.jl, test/int.jl for both x::Signed % Unsigned and x::Unsigned % Signed.
When this builds, and there are no further notes, please mash-as-one these commits.

Keno · 2020-02-25T05:10:14Z

test/int.jl

@@ -266,6 +266,14 @@ end
    U in [Base.BitInteger_types..., BigInt]
    @test typeof(rand(U(0):U(127)) % T) === T
 end
+@testset "x::Signed % Unsigned returns a T, T = $T" for T in [Base.BitSigned_types...]
+    @test typeof(rand(T) % Unsigned) <: Unsigned


I'd rather this test a few interesting cases instead (or in addition). For example typemax, typemin, -typemax. Also should test for x a BigInt, since that's an interesting one for which there is no unsigned type.

What do you think? Should BigInt(x) % Unsigned always throw an error, or should it handle values that only require 128 signed bits and throw and error for larger values?

I was surprised that unsigned(Int64) == UInt64 but signed(UInt64) errors. We should remedy that, defining signed(x::T) where {T<:Base.BitUnsigned_types} to be the size corresponding BitSigned_types. I am making that change in int.jl as these BitInteger pairs should be with the BitInteger, BitSigned, BitUnsigned type declarations.
Half of them, the unsigned(T) where {T<:BitSigned} are given in multinverse.jl, which is its own module. I am removing those lines from that file and adding unsigned to the short list Base sourced things given with using Base:.

Why support x::BigInt % Unsigned? It doesn't really make sense, because BigInt is not a bitstype and these conversions are from one n-byte integer type (signed or unsigned) to another n-byte integer type (unsigned or signed).
The error that happens .. no method matching Unsigned(::BigInt) seems appropriate,

Fixed-size integer types represent modular classes of integer values. When you do x % T you are saying "give me the element of that represents the modular class of x in T. That is a perfectly well-defined operation for x::BigInt % Unsigned. In fact, this operation is always well-defined because x represents a specific integer value which must belongs to exactly one modular class in T and the result is the unique value in T representing that class.

But in x::BigInt % Unsigned, how to choose the destination type? We won't define an UnsignedBigInt type. And we can't return x because it's expected that (x % Unsigned) isa Unsigned.

Oh, I see what you mean—I interpreted that as being for some specific T <: Unsigned but the issue is for the literal type Unsigned. That is quite unclear so agree about it being an error. The alternative would be to choose UInt as the "default unsigned type" but that's questionable.

The alternative would be to choose UInt as the "default unsigned type" but that's questionable.

I guess it's always possible to add it later if there is demand for that, but while this makes some sense, this seems a bit weird to return a smaller type than Int128(1) % Unsigned.

Agree. Which leads to an issue if/when we ever add (U)Int256 types.

JeffreySarnoff · 2020-02-25T16:47:05Z

With Base and this PR with or without (U)Int256 bitstypes:

x::Signed
- x === x % Signed
x::Unsigned
- x % Unsigned
x::BitInteger
- isa(x % Signed, Signed) & isa(x % Unsigned, Unsigned)
- sizeof(x) === sizeof(x % Signed) === sizeof(x % Unsigned)
x::BitUnsigned
- isa(x, Unsigned) & isa(x, BitInteger)
- x === x % Signed % Unsigned
x::BitSigned
- isa(x, Signed) & isa(x, BitInteger)
- x === x % Unsigned % Signed

A package exporting a new pair of Signed and Unsigned integer types (S and U) can follow suit by defining a corresponding pair of convert functions and adding

Base.unsigned(S) = U
Base.signed(U) = S
Base.rem(x::S, ::Type{U}) = convert(U, x)
Base.rem(x::U, ::Type{S}) = convert(S, x)

(this works for bitstypes, [mutable] structs, and external library types)

Keno · 2020-02-25T20:01:54Z

I think it's worth discussing this on triage, particularly for the BigInt case.

Keno · 2020-02-27T19:23:00Z

base/int.jl

@@ -44,6 +44,22 @@ const BitUnsigned64T   = Union{Type{UInt8}, Type{UInt16}, Type{UInt32}, Type{UIn

 const BitIntegerType = Union{map(T->Type{T}, BitInteger_types)...}

+# interconvert equilength BitInteger types
+
+unsigned(::Type{Int8}) = UInt8


These should already be defined. Why is this necessary?

unsigned(::Type{IntN}) = UIntN are defined in Base/multinverses.jl.
signed(::Type{UIntN}) = IntN are not defined.
I thought the two sets of definitions belonged in the same place, and that Int.jl was more appropriate than multinverses.jl. I moved the defs from multinverses.jl to Int.jl and added the defs for signed(_) there. I added unsigned to the using Base: line in multinverses.jl.

I can revert this and I could add the signed(_) defs into multinverses.jl. multiinverses is a module, would I need to export signed after adding it there?

all that is undone

NEWS.md

Co-Authored-By: Matt Bauman <mbauman@gmail.com>

JeffreySarnoff · 2020-03-05T08:45:09Z

😄

JeffreySarnoff · 2020-03-06T02:47:27Z

error is unrelated

Error in testset Serialization:
Error During Test at none:1
  Test threw exception
  Expression: Serialization
  ProcessExitedException(3)

StefanKarpinski · 2020-03-27T14:00:37Z

What's the status of this? Done? Waiting for CI to pass?

JeffreySarnoff · 2020-03-27T14:13:39Z

This is done and it is good to go now

rfourquet · 2020-03-27T14:21:44Z

NEWS.md

@@ -92,6 +92,9 @@ New library features

 * `isapprox` (or `≈`) now has a one-argument "curried" method `isapprox(x)` which returns a function, like `isequal` (or `==`)` ([#32305]).
 * `Ref{NTuple{N,T}}` can be passed to `Ptr{T}`/`Ref{T}` `ccall` signatures ([#34199])
+* `accumulate`, `cumsum`, and `cumprod` now support `Tuple` ([#34654]).


Delete this line, which must come from rebasing (the correct version is indicated 3 lines below)

rfourquet · 2020-03-27T14:25:54Z

test/int.jl

+        @test unsigned(S) === U
+    end
+end
+@testset "x::[Un]Signed % [Un]Signed returns x" begin


Just personal taste, but I would join the three @testsets together, as they contain the same loop, so it's a bit of a ceremony overhead.

rfourquet · 2020-03-27T14:28:38Z

This might need updating the docstrings (I know that unsigned(::Type) is currently undocumented, but this PR publicizes the feature in the NEWS.md).

JeffreySarnoff · 2020-03-27T14:41:03Z

arrgh -- ok about the doc strings --

JeffreySarnoff · 2020-03-27T15:28:39Z

I added the docstring for signed(T) where e.g signed(::Type{UInt8}) = Int8 appears.
I do not know where the corresponding unsigned(T) are, so I added that docstring before the signed(T) section using """ ... """ unsigned.

JeffreySarnoff · 2020-03-27T19:50:53Z

@StefanKarpinski there it be ✅ .. please merge before something else changes :))

JeffreySarnoff added 2 commits February 24, 2020 18:58

support x % Unsigned where isa(x, Signed)

6a35def

This follows from a discussion on Slack; the implementation was suggested by @Keno.

support x % Unsigned where isa(x, Signed)

6b87b76

Keno reviewed Feb 25, 2020

View reviewed changes

Keno added the needs tests Unit tests are required for this change label Feb 25, 2020

JeffreySarnoff added 5 commits February 24, 2020 19:30

support x % Unsigned where isa(x, Signed)

17f3c79

x % Unsigned where isa(x, Signed) (remove comma)

58f8708

x::Signed % Unsigned, x::Unsigned % Signed

65367c5

rem(x::Unsigned, Signed)

1a1b028

x::Unsigned % Signed

bcaa805

remove trailing space

fb30b63

JeffreySarnoff changed the title ~~rem(x::Signed, ::Type{Unsigned})~~ tests for x::Signed % Unsigned Feb 25, 2020

JeffreySarnoff changed the title ~~tests for x::Signed % Unsigned~~ tests for x::Signed % Unsigned Feb 25, 2020

JeffreySarnoff changed the title ~~tests for x::Signed % Unsigned~~ x % Unsigned, x % Signed Feb 25, 2020

Keno reviewed Feb 25, 2020

View reviewed changes

Keno removed the needs tests Unit tests are required for this change label Feb 25, 2020

JeffreySarnoff added 9 commits February 25, 2020 00:40

interconvert BitInteger pairs

bfd416b

remove unsigned(<:Signed) lines, import unsigned from Base

536948e

add tests, improve tests

152d6e8

news about signed(T), unsigned(T)

98204f5

remove trailing whitespace

c1988c1

fix test

61e2362

fix missing begin

3fbcf18

add missing end

a1da6f4

revise tests

8542442

fixup typo in test

9172d4d

Keno added the triage This should be discussed on a triage call label Feb 25, 2020

Keno reviewed Feb 27, 2020

View reviewed changes

mbauman reviewed Mar 3, 2020

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

JeffreySarnoff and others added 2 commits March 3, 2020 16:24

Update NEWS.md

a67e742

Co-Authored-By: Matt Bauman <mbauman@gmail.com>

Merge branch 'master' into jas/remUnsigned

cd61928

JeffreySarnoff added 6 commits March 14, 2020 00:54

remove signed(UIntNN) = IntNN (does it fix?)

ecb9966

remove signed(UIntNN) == IntNN tests (does it fix?)

3cbbec4

restore signed(::Type{UIntNN}) = IntNN

574d8a6

restore signed(::Type{UIntNN}) = IntNN tests

100884e

whitespace

cf1954a

Merge branch 'master' into jas/remUnsigned

03891ac

rfourquet reviewed Mar 27, 2020

View reviewed changes

JeffreySarnoff added 2 commits March 27, 2020 10:30

remove old line

6dc7fab

combine 3 testsets

9c2befc

JeffreySarnoff added 2 commits March 27, 2020 11:20

docstrings for signed(T) unsigned(T)

7b00bd5

move docstring up

f46360a

StefanKarpinski merged commit d31b047 into JuliaLang:master Mar 27, 2020

JeffreySarnoff deleted the jas/remUnsigned branch March 27, 2020 20:07

oxinabox pushed a commit to oxinabox/julia that referenced this pull request Apr 8, 2020

x % Unsigned, x % Signed (JuliaLang#34864)

f3502f8

ravibitsgoa pushed a commit to ravibitsgoa/julia that referenced this pull request Apr 9, 2020

x % Unsigned, x % Signed (JuliaLang#34864)

987d664

KristofferC pushed a commit that referenced this pull request Apr 11, 2020

x % Unsigned, x % Signed (#34864)

2658ad5

kimikage mentioned this pull request Jan 21, 2021

Add signed(T), unsigned(T). #30445

Closed

NHDaly mentioned this pull request Apr 13, 2021

unsigned(::Type) and signed(::Type) #30444

Closed

NHDaly mentioned this pull request Feb 17, 2022

Add unsigned(T) in addition to existing unsigned(x::XBS). rfourquet/BitIntegers.jl#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x % Unsigned, x % Signed #34864

x % Unsigned, x % Signed #34864

JeffreySarnoff commented Feb 25, 2020

Keno Feb 25, 2020

JeffreySarnoff Feb 25, 2020

JeffreySarnoff commented Feb 25, 2020

Keno Feb 25, 2020

JeffreySarnoff Feb 25, 2020

JeffreySarnoff Feb 25, 2020 •

edited

Loading

JeffreySarnoff Feb 25, 2020

StefanKarpinski Feb 25, 2020 •

edited

Loading

rfourquet Feb 25, 2020

StefanKarpinski Feb 25, 2020

rfourquet Feb 25, 2020

StefanKarpinski Feb 25, 2020

JeffreySarnoff commented Feb 25, 2020 •

edited

Loading

Keno commented Feb 25, 2020

Keno Feb 27, 2020

JeffreySarnoff Feb 29, 2020 •

edited

Loading

JeffreySarnoff Mar 15, 2020

JeffreySarnoff commented Mar 5, 2020

JeffreySarnoff commented Mar 6, 2020 •

edited

Loading

StefanKarpinski commented Mar 27, 2020

JeffreySarnoff commented Mar 27, 2020

rfourquet Mar 27, 2020

rfourquet Mar 27, 2020

rfourquet commented Mar 27, 2020

JeffreySarnoff commented Mar 27, 2020 •

edited

Loading

JeffreySarnoff commented Mar 27, 2020

JeffreySarnoff commented Mar 27, 2020

x % Unsigned, x % Signed #34864

x % Unsigned, x % Signed #34864

Conversation

JeffreySarnoff commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffreySarnoff commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffreySarnoff Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffreySarnoff commented Feb 25, 2020 • edited Loading

Keno commented Feb 25, 2020

Choose a reason for hiding this comment

JeffreySarnoff Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffreySarnoff commented Mar 5, 2020

JeffreySarnoff commented Mar 6, 2020 • edited Loading

StefanKarpinski commented Mar 27, 2020

JeffreySarnoff commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfourquet commented Mar 27, 2020

JeffreySarnoff commented Mar 27, 2020 • edited Loading

JeffreySarnoff commented Mar 27, 2020

JeffreySarnoff commented Mar 27, 2020

JeffreySarnoff Feb 25, 2020 •

edited

Loading

StefanKarpinski Feb 25, 2020 •

edited

Loading

JeffreySarnoff commented Feb 25, 2020 •

edited

Loading

JeffreySarnoff Feb 29, 2020 •

edited

Loading

JeffreySarnoff commented Mar 6, 2020 •

edited

Loading

JeffreySarnoff commented Mar 27, 2020 •

edited

Loading