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

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

Merged
merged 9 commits into from
Feb 6, 2017

Conversation

stevengj
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

base/number.jl Outdated
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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@stevengj stevengj Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@stevengj stevengj Jan 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

base/number.jl Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

base/number.jl Outdated
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

base/number.jl Outdated
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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x::Real should make -1 okay here.

@timholy
Copy link
Member

timholy commented Jan 27, 2017

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

@stevengj
Copy link
Member Author

stevengj commented Jan 27, 2017

@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.

@kshyatt kshyatt added the maths Mathematical functions label Jan 27, 2017
base/array.jl Outdated
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't you be passing x to these?

@ajkeller34
Copy link
Contributor

ajkeller34 commented Jan 31, 2017

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
Copy link
Member Author

stevengj commented Jan 31, 2017

@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
Copy link
Contributor

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
Copy link
Contributor

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

@stevengj
Copy link
Member Author

stevengj commented Feb 2, 2017

(Rebased.) @timholy, any further comments?

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stevengj
Copy link
Member Author

stevengj commented Feb 4, 2017

Rebased. Good to merge when green?

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shame to mess with the comment spacing in the style guide

@timholy
Copy link
Member

timholy commented Feb 5, 2017

Thanks for tackling this, @stevengj.

@timholy
Copy link
Member

timholy commented Feb 5, 2017

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

@stevengj
Copy link
Member Author

stevengj commented Feb 6, 2017

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

@stevengj stevengj merged commit 7fb758a into JuliaLang:master Feb 6, 2017
@stevengj stevengj deleted the oneunit branch February 6, 2017 00:20
@timholy
Copy link
Member

timholy commented Feb 6, 2017

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

mdavezac pushed a commit to mdavezac/julia that referenced this pull request Sep 13, 2017
According to docstring, `ones` creates an array of `T`, hence it should
call `oneunit` rather than `one`. See JuliaLang#16116 and JuliaLang#20268.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants