Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Int8(1) instead of true in I #29596

Closed
wants to merge 1 commit into from
Closed

Use Int8(1) instead of true in I #29596

wants to merge 1 commit into from

Conversation

andreasnoack
Copy link
Member

This is a slightly modified version of #24396 to make I use Int8 instead of Bool. For almost all real use cases, I would expect the behavior to be unaffected by this change since Int8 and Bool have the same promotion behavior in almost all the relevant cases. In particular, they have the same promotion behavior in the cases mentioned in #24396.

The main motivation for this change is that

julia> Matrix(I, 3, 3)
3×3 Array{Bool,2}:
  true  false  false
 false   true  false
 false  false   true

is not really what you'd expect from an identity matrix. With this change, it would instead be

julia> Matrix(I, 3, 3)
3×3 Array{Int8,2}:
 1  0  0
 0  1  0
 0  0  1

I'd be interested in a PkgEval run of this since I wouldn't expect any user code to break from this change.

@andreasnoack andreasnoack added the linear algebra Linear algebra label Oct 11, 2018
@chethega
Copy link
Contributor

julia> M1=Int8[1 0 0; 0 1 0; 0 0 1];
julia> M2 = Bool[1 0 0; 0 1 0; 0 0 1];
julia> v=[1, 2, NaN];
julia> isequal(M1*v, v)
false
julia> isequal(M2*v, v)
true

I think that hard zeros are appropriate for the identity matrix.

@StefanKarpinski
Copy link
Sponsor Member

I don't really love it. Couldn't we just change the behavior of Matrix(I, 3, 3)?

@andreasnoack
Copy link
Member Author

andreasnoack commented Oct 11, 2018

I think that hard zeros are appropriate for the identity matrix.

Could you come up with an example that actually uses I? I seriously doubt that anybody would rely on this. It would have to be something something like

julia> false*I*[Inf, NaN]
2-element Array{Float64,1}:
 0.0
 0.0

Do you really multiply the identity with a Boolean? It seems like a pretty rare operation to me.

I don't really love it. Couldn't we just change the behavior of Matrix(I, 3, 3)?

I think that would be a bit odd if Matrix(I, 3, 3) doesn't use the element type of I. Even if we made that change, we'd still have

julia> I[2,1]
false

julia> I[1,1]
true

and I think it's odd that the identity doesn't contain zeros and ones. Notice that for the first many years, the element type of I was Int and this was only changed last year to improve promotion for small types.

@chethega
Copy link
Contributor

chethega commented Oct 11, 2018

Could you come up with an example that actually uses I?

I think that

julia> v=[1.0, NaN];
julia> isequal(Matrix(I,2,2)*v, v)
true

is really what you'd expect from an identity matrix. Your changed identity matrix does not act by identity modulo isequal on floating point vectors containing NaN or infinities.

If users are surprised by the display, i.e. that a dense matrix acting by identity on vectors of Floats containing NaN/Inf can only be represented over Bool, that is imho no compelling reason to change behavior by stopping to let the officially exported identity matrix act by identity on such vectors.

If it is instead decided that off-diagonal zeros must be soft, for consistency with BLAS libraries, and return an all-NaN vector when multiplied to a Inf/NaN-containing vector, then that is a very compelling reason to do this change (that has nothing to do with pretty printing).

PS. No, I do not have examples where anybody should rely on this.

@andyferris
Copy link
Member

Is this more of a symptom of the fact that Bool almost means UInt1 but we’re not quite committed to that? Why does Bool get “hard” zero semantics under multiplication, when Int8 doesn’t?

Basically... we left Bool on the fence in #19168. I kind of agree that Bool here has a few problems which are fixed by Int8 and there aren’t really any practical drawbacks, so this seems fine. But then I also wonder if we need a “real” one-bit number type distinct from a logical / Boolean type...

@andreasnoack
Copy link
Member Author

Why does Bool get “hard” zero semantics under multiplication, when Int8 doesn’t?

My understanding is that this was motivated by #5468

@andyferris
Copy link
Member

Cool, thanks for the ref.

#5468 went to the effort of adding a check of the sign to give -0.0 in the correct cases - I suppose to preserve standard IEEE semantics. I would similarly propose another check on !isfinite(y) to result in a nan value for the same purpose.

@JeffBezanson
Copy link
Sponsor Member

Maybe we could print Bools as 0 and 1 when the typeinfo context allows it?

@nalimilan
Copy link
Member

That would be confusing for people who really want Boolean values and not numbers.

@andyferris
Copy link
Member

andyferris commented Oct 13, 2018

@nalimilan I sympathise - but it has to be said, given we have Bool <: Number that quite natural phrase is kind of impossible...

@mschauer
Copy link
Contributor

I don't think the following would be very confusing, the type is prominently stated. It is rather an improvement in readability.

julia> rand(Bool, 3,3)
3×3 Array{Bool,2}:
 0  1  0
 1  0  1
 0  0  0

@chethega
Copy link
Contributor

On the other hand,

julia> 0 && print("0")
ERROR: TypeError: non-boolean (Int64) used in boolean context

So yes, we have Bool <: Number, but we are pretty strict in forbidding 0 or 1 as identifiers for true or false. I understand why: prevent people from being too terse and better errors than e.g. ERROR: InexactError: Bool(Bool, 2).

I'd still like the more terse output of Bool-arrays as 0/1.

@StefanKarpinski
Copy link
Sponsor Member

Note that we already print some integer types in a way that doesn't round trip to the same type, e.g.:

julia> Int8(123)
123

julia> rand(Int8, 5, 5)
5×5 Array{Int8,2}:
 -18  -84  -101   57   65
  80   89  -101  107  -68
  15  -88   -14  110  -51
 -21  -93   100   21  -83
 -86  -41  -109    7  -80

If you want the actual same value back you can do T(123) which works with T as Bool as well.

@ararslan
Copy link
Member

Note that we already print some integer types in a way that doesn't round trip to the same type

True, but that seems like more of an unfortunate consequence of not having literals for integer types like Int8 than actually desirable behavior. It seems to me that if we can print something as what it is, we should. It makes very little sense to me to change how Bools are shown when we already have a way to represent them.

@ararslan
Copy link
Member

So yes, we have Bool <: Number, but we are pretty strict in forbidding 0 or 1 as identifiers for true or false.

This is a very good point. They are not generally interchangeable, so we should not print them as though they are.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Oct 15, 2018

It makes very little sense to me to change how Bools are shown when we already have a way to represent them.

This is fair point but the thing is that it's struck me many times for entirely different reasons that the printing of boolean vectors and matrices is really annoying and verbose, so I already want to not print them using the standard input syntax for true and false. Meanwhile, Bool[1,0,1,1,0] and Bool[1 0; 1 1] are perfectly valid input syntaxes for a boolean vector and matrices.

@andreasnoack
Copy link
Member Author

Just curious about the negative feedback here since I considered this change extremely small.

For a long time I=UniformScaling(1) and people were happy. Then we realized that this promoted unnecessarily in some cases for small integer matrices and to avoid that (an only that) we changed it to I=UniformScaling(true). This PR changes this to something that for most practical purposes promotes similarly to true but has the advantage that it actually looks like an identity matrix.

So, I'd like to hear why people are sceptical here. (And please keep the strong zero property out of the discussion because it never played a role in the first place). Changing the printing of Boolean matrices might also be a fine solution here but I'm still curious why this is considered so bad.

@andyferris
Copy link
Member

The only remaining thing that occurs to me is that sometimes you want to study F2 - aka GF(2) - akaUInt1 without widening promotion by default, where 1 + 1 = 0. This is a fine number type (it’s a field) and I’d expect that our generic linear algebra libraries to work just fine on all types that are fields and subtype of Number. This takes us slightly away from that (making I slightly less likely to do what you expect when dealing with GF(2)).

But that’s pretty esoteric for many users so not sure - this PR has other small advantages and we don’t actually have a number type that faithfully represents GF(2) at the moment.

@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Oct 16, 2018

Just curious about the negative feedback here since I considered this change extremely small.

Because it goes from using an element type that is explicitly intended to be "weak" in the sense of always yielding to others when promoted to one that just happens to be weaker than most other types most of the time because it just happens to print in a way that people prefer. That's a lot of "just happens"—which bothers me.

If people find the printing of Matrix{Bool} so objectionable—and I do find it annoying—then why don't we just change that? If you don't find it objectionable then the entire motivation of this issue should be moot: you shouldn't have any problem with the way Matrix(I, 3, 3) prints. There's no problem with the behavior of I here, just the printing when used to fill another matrix. Changing the behavior to something less correct just to get better printing rather than just improving the printing really rubs me the wrong way.

@JeffBezanson
Copy link
Sponsor Member

Agree, it seems much easier to read to print Bool matrices and vectors with 0 and 1.

@mbauman
Copy link
Sponsor Member

mbauman commented Oct 16, 2018

@andreasnoack Am I correct in understanding here that you don't object to any of the behaviors of UniformScaling{Bool} besides the fact that seeing true and false feels weird by default?

@andreasnoack
Copy link
Member Author

@mbauman Yes, the idea was just to make the identity matrix have number elements instead of word elements and to do that with a minimal change that wouldn't have any (real) behavioral effects. As mentioned in the top post, I'd be very surprised if this PR would break any user code.

I didn't consider the possibility of changing the printing of Array{Bool} before preparing this PR. I assume that we wouldn't change scalar Booleans anyway so we'd still have the unfortunate printing when indexing into I, e.g.

julia> I[1,1]
true

@andreasnoack
Copy link
Member Author

Superseded by #30575

@andreasnoack andreasnoack deleted the an/I branch January 4, 2019 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants