WIP: add oneunit(x) for dimensionful version of one(x) #20268

stevengj · 2017-01-27T03:33:08Z

Following the discussion in #16116, this clarifies the documentation of one(x) to indicate that, since it is the multiplicative identity, it may not be the of the same type as x for types representing dimensionful quantities. It defines the function oneunit(x::T) = T(one(x)) for when you need the same type as x and x might be dimensionful.

I also went through Base and changed one -> oneunit where it seemed appropriate. I didn't bother with usages of one in functions where a dimensionful quantity would not be meaningful, e.g. exp(x), or where the type was otherwise constrained.

This change is not sufficient to make all of Base work with dimensionful quantities, but it is a start. It would be good to have a test/dimensionful.jl test suite that began to exercise various base functions with dimensionful quantities (e.g. a minimalist version of SIUnits.jl).

vchuravy · 2017-01-27T05:43:54Z

I find oneunit quite a mouthful and I like oneof better. (as in one of Type T). But that is personal taste and I don't want to distract with syntax bike shedding.

+1 to introducing this concept.

nalimilan · 2017-01-27T08:49:24Z

Is a new function really needed since T(1) and oftype(x, 1) are quite short? (+1 for the changes in general though.)

timholy

👍

timholy · 2017-01-27T11:17:34Z

base/number.jl

-Get the multiplicative identity element for the type of `x` (`x` can also specify the type
-itself). For matrices, returns an identity matrix of the appropriate size and type.
+Return a multiplicative identity for `x` (which can be either a value
+or a type), of the same shape and numeric precision as `x`.  (For matrices,


I'm pretty sure that most people disliked the "shape" part, and that most think thought one on Matrix{Float64} should return a number (1.0, not eye(n,n)).

This PR doesn't have to do the deprecation, but I don't think we should put "shape" into the docstring.

I didn't get the sense that there was any kind of consensus on whether one(matrix) should stop returning a matrix. From an algebraic perspective, returning a matrix makes more sense. It is also the right thing to do from the perspective of ^ and prod, both of which need the identity matrix for an empty product.

Moreover, oneunit(x) only makes sense for matrices if one returns a matrix, since for oneunit(x::T) you have to be able to do T(one(x)), and this will fail if T::Matrix and one(x)::Number.

My perspective is that (a) oneunit(x::T) should be guaranteed to return something of type T and (b) one and oneunit should give the same result except for types representing dimensionful quantities (i.e. except for cases where the multiplicative identity cannot be the same type).

It is also the right thing to do from the perspective of ^ and prod, both of which need the identity matrix for an empty product.

Not sure I see how, since one(Matrix{Float64}) already doesn't work---you need at least one "sized" object to know what to return.

You're right that there are competing arguments. If you return a scalar, then one works in both the type-domain and the value-domain, whereas oneunit can't be defined. If you return a matrix, then you can't define one or oneunit in the type-domain.

I mostly think of these as "traits" and therefore the type-domain wins (not even a contest). But I recognize that this viewpoint is debatable.

@timholy, it works for M^0, since then it can return one(M). It would also work for an empty product of SMatrix, since then the size is encoded in the type.

timholy · 2017-01-27T11:19:55Z

base/number.jl

@@ -133,11 +133,41 @@ zero{T<:Number}(::Type{T}) = convert(T,0)
 """
    one(x)

-Get the multiplicative identity element for the type of `x` (`x` can also specify the type
-itself). For matrices, returns an identity matrix of the appropriate size and type.
+Return a multiplicative identity for `x` (which can be either a value


"multiplicative identity" may not be clear to everyone, better to somewhere say "such that one(x)*x == x*one(x) == x".

timholy · 2017-01-27T11:23:22Z

base/number.jl

+oneunit{T}(x::T) = T(one(x))
+oneunit{T}(::Type{T}) = T(one(T))
+
+dimensionless{T<:Number}(::Type{T}) = typeof(one(T)) # convert T to dimensionless type


You don't use this anywhere, and the name does not make it clear whether this would return a value or a type. Delete?

timholy · 2017-01-27T11:30:38Z

base/number.jl

-sign(x::Real) = ifelse(x < 0, oftype(x,-1), ifelse(x > 0, one(x), x))
-sign(x::Unsigned) = ifelse(x > 0, one(x), x)
+sign(x::Number) = x == 0 ? x/abs(oneunit(x)) : x/abs(x)
+sign(x::Real) = ifelse(x < 0, oftype(one(x),-1), ifelse(x > 0, one(x), oftype(one(x),0)))


oftype(one(x), -one(x)) might be safer than oftype(one(x), -1).

x::Real should make -1 okay here.

timholy · 2017-01-27T11:37:17Z

@nalimilan, the voting (#16116 (comment)) was almost evenly split on this issue.

stevengj · 2017-01-27T16:22:06Z

@nalimilan, oftype(x, 1) is wrong here because it assumes that 1 is convertible to the type of x, which is not true for dimensionful quantities (e.g. oftype(Dates.Day(3), 1) fails), or quantities that are not numeric scalars but which still have an algebra (e.g. matrices). In any case, I think oneunit is clearer to read (better at indicating the programmer's intentions), and it is also much easier to do a search-and-replace of one with oneunit to fix up usages.

@vchuravy, I think the name oneunit is better than oneof at indicating that this function differs from one mainly for dealing with dimensionful quantities.

tkelman · 2017-01-28T05:38:06Z

base/array.jl

    m,n = size(x)
    m==n || throw(DimensionMismatch("multiplicative identity defined only for square matrices"))
    eye(T, m)
 end

+one{T}(x::AbstractMatrix{T}) = _one(one(T))
+oneunit{T}(x::AbstractMatrix{T}) = _one(oneunit(T))


shouldn't you be passing x to these?

ajkeller34 · 2017-01-31T22:55:31Z

You could use the new definition of eye with a dimensionful type and wind up not creating the identity matrix for that type. In other words, if you had two matrices with 1.0m along the diagonal and 0.0m elsewhere, upon multiplication you would end up with a matrix having 1.0m^2 along the diagonal. The true identity matrix for [1.0m 0.0m; 0.0m 1.0m] would be [1.0 0.0; 0.0 1.0].

It's not clear to me that this is the right way to go, but if this change is kept, perhaps we could reword the docstring for eye to something in the spirit of, but less clumsy than: "Constructs a matrix with the same dimensions and type as A, where eltype(A)(1) is along the diagonal and eltype(A)(0) is elsewhere. For dimensionless numbers, this is an identity matrix, but not in general." An alternative choice would be "Constructs an identity matrix of the same dimensions as A (and of the same type, where possible)."

edit: by alternative choice I mean the docstring for an alternative implementation of eye.

stevengj · 2017-01-31T23:58:27Z

@ajkeller34, if you call eye(X) on a dimensionful matrix X, the new definition should return the correct (dimensionless) multiplicative identity for X (assuming one is defined correctly). If you call eye(T, m,n), it seems to me that it should create a Matrix{T}, however, even if T is dimensionful.

ajkeller34 · 2017-02-01T00:25:03Z

I agree with you, I think. To be clear though, are you saying you are fine with eye(T,m,n) not returning an identity matrix for dimensionful T? Or would you rather eye(T,m,n) fail for dimensionful T, so that eye will only ever return an identity matrix?

ajkeller34 · 2017-02-01T00:29:10Z

Ah, never mind, didn't see your updated comment.

stevengj · 2017-02-02T16:08:25Z

(Rebased.) @timholy, any further comments?

timholy

LGTM

and change one -> oneunit where appropriate.

stevengj · 2017-02-04T00:16:16Z

Rebased. Good to merge when green?

tkelman · 2017-02-04T09:29:17Z

doc/src/manual/style-guide.md

-addone(x::Integer) = x + one(x)    # any integer type
-addone(x::Number) = x + one(x)     # any numeric type
-addone(x) = x + one(x)             # any type supporting + and one
+addone(x::Integer) = x + oneunit(x)    # any integer type


shame to mess with the comment spacing in the style guide

timholy · 2017-02-05T23:50:14Z

Thanks for tackling this, @stevengj.

timholy · 2017-02-05T23:51:51Z

Oh, maybe I'll let you merge. I'd prefer to squash this down, "whoops" is not the most informative commit message 😄.

stevengj · 2017-02-06T00:18:45Z

@timholy, the "squash and merge" button lets you edit the commit message. I usually trim out all the extraneous commit messages at that point.

timholy · 2017-02-06T10:10:04Z

I'm well aware, I just didn't want to take liberties in case there was a reason to have separate commits.

According to docstring, `ones` creates an array of `T`, hence it should call `oneunit` rather than `one`. See JuliaLang#16116 and JuliaLang#20268.

stevengj mentioned this pull request Jan 27, 2017

things we should deprecate, 0.6 edition #19598

Closed

22 tasks

timholy reviewed Jan 27, 2017

View reviewed changes

kshyatt added the maths Mathematical functions label Jan 27, 2017

tkelman reviewed Jan 28, 2017

View reviewed changes

tkelman mentioned this pull request Feb 1, 2017

RFC: Modifying base to play nicely with physical units #15394

Closed

stevengj force-pushed the oneunit branch from 09c1375 to 2fe9198 Compare February 2, 2017 16:07

timholy approved these changes Feb 2, 2017

View reviewed changes

waldyrious mentioned this pull request Feb 2, 2017

trig functions are not type-stable for complex inputs JuliaMath/Tau.jl#14

Closed

stevengj and others added 8 commits February 3, 2017 19:15

add oneunit(x) for dimensionful version of one(x),

cd09a37

and change one -> oneunit where appropriate.

import oneunit into LinAlg

b2ccdf3

fix eye and one for matrices to use oneunit

ab318c7

rm unused function dimensionless

b1434e7

one/oneunit doc improvements

e2e6f25

NEWS

34a82e1

whoops

5c46b0d

fix sign function for NaN

c1d6e06

stevengj force-pushed the oneunit branch from 2fe9198 to c1d6e06 Compare February 4, 2017 00:15

tkelman reviewed Feb 4, 2017

View reviewed changes

fix spacing in style guide

20dbdef

stevengj merged commit 7fb758a into JuliaLang:master Feb 6, 2017

stevengj deleted the oneunit branch February 6, 2017 00:20

stevengj mentioned this pull request Feb 6, 2017

Disambiguate the meaning of one #16116

Closed

mdavezac mentioned this pull request Sep 1, 2017

ones should call oneunit #23544

Closed

mdavezac pushed a commit to mdavezac/julia that referenced this pull request Sep 13, 2017

ones should call oneunit

6fdbda3

According to docstring, `ones` creates an array of `T`, hence it should call `oneunit` rather than `one`. See JuliaLang#16116 and JuliaLang#20268.

Sacha0 mentioned this pull request Nov 13, 2017

design of array constructors #24595

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: add oneunit(x) for dimensionful version of one(x) #20268

WIP: add oneunit(x) for dimensionful version of one(x) #20268

stevengj commented Jan 27, 2017

vchuravy commented Jan 27, 2017

nalimilan commented Jan 27, 2017

timholy left a comment

timholy Jan 27, 2017

stevengj Jan 27, 2017 •

edited

Loading

stevengj Jan 27, 2017

timholy Jan 27, 2017

stevengj Jan 27, 2017 •

edited

Loading

timholy Jan 27, 2017

timholy Jan 27, 2017

timholy Jan 27, 2017

stevengj Jan 27, 2017

timholy commented Jan 27, 2017

stevengj commented Jan 27, 2017 •

edited

Loading

tkelman Jan 28, 2017

ajkeller34 commented Jan 31, 2017 •

edited

Loading

stevengj commented Jan 31, 2017 •

edited

Loading

ajkeller34 commented Feb 1, 2017

ajkeller34 commented Feb 1, 2017

stevengj commented Feb 2, 2017 •

edited

Loading

timholy left a comment

stevengj commented Feb 4, 2017

tkelman Feb 4, 2017

timholy commented Feb 5, 2017

timholy commented Feb 5, 2017

stevengj commented Feb 6, 2017

timholy commented Feb 6, 2017

WIP: add oneunit(x) for dimensionful version of one(x) #20268

WIP: add oneunit(x) for dimensionful version of one(x) #20268

Conversation

stevengj commented Jan 27, 2017

vchuravy commented Jan 27, 2017

nalimilan commented Jan 27, 2017

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Jan 27, 2017 • 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

timholy commented Jan 27, 2017

stevengj commented Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

ajkeller34 commented Jan 31, 2017 • edited Loading

stevengj commented Jan 31, 2017 • edited Loading

ajkeller34 commented Feb 1, 2017

ajkeller34 commented Feb 1, 2017

stevengj commented Feb 2, 2017 • edited Loading

timholy left a comment

Choose a reason for hiding this comment

stevengj commented Feb 4, 2017

Choose a reason for hiding this comment

timholy commented Feb 5, 2017

timholy commented Feb 5, 2017

stevengj commented Feb 6, 2017

timholy commented Feb 6, 2017

stevengj Jan 27, 2017 •

edited

Loading

stevengj Jan 27, 2017 •

edited

Loading

stevengj commented Jan 27, 2017 •

edited

Loading

ajkeller34 commented Jan 31, 2017 •

edited

Loading

stevengj commented Jan 31, 2017 •

edited

Loading

stevengj commented Feb 2, 2017 •

edited

Loading