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

Restrict zero and one to Number types. Default to floating points. #6183

Merged
merged 1 commit into from Apr 4, 2014

Conversation

andreasnoack
Copy link
Member

There is some room for choosing the return type of e.g. zero(Real) and right now the default is Int. By using floating points instead the InexactError in #4796 could have been avoided. This pr also restricts zero and one such that zero/one(Any) is no longer defined. It is possible that this change will break some code with type inference problems, but maybe that is a good.

@andreasnoack
Copy link
Member Author

Okay. Now the pull request actually works. I understand that it is better to start out with integers when converting to other types, but I think it would be useful if one(Real)=1.0 and one(Complex)=1.0+1.0im.

@andrioni
Copy link
Member

Minor comment: do you mind defining zero(BigInt) and one(BigInt) in gmp.jl instead of mpfr.jl?

@Jutho
Copy link
Contributor

Jutho commented Mar 17, 2014

Do you mean to restrict zero and one to subtypes of Number, i.e. scalar quantities. Does this mean there will be no zero for arrays or one for square matrices?

@andreasnoack
Copy link
Member Author

@andrioni No of course not. I'll change that. For some reason, I thought that all the big stuff was in mpfr.jl now.

@Jutho You can still get

julia> one(randn(3,3))
3x3 Array{Float64,2}:
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

but not one(Any)=1, which was the main thing I'd like to restrict.

@andreasnoack
Copy link
Member Author

@tkelman I can see that you wrote the sparse test about "access to undefined error". I don't recall the idea of this test. I have change the array type from Any to Number. Does that make sense?

@tkelman
Copy link
Contributor

tkelman commented Mar 17, 2014

@andreasnoackjensen I believe so. Since Array(Any,n) and Array(Number,n) both get allocated as arrays of #undef, they would both trigger the issue in the way sparse matmul and sparse diff formerly used splice! (PR was #5852).

splice!(Array(Number,5),5) gives the same error that splice!(Array(Any,5),5) does.

deleteat!(Array(Number,5),5) and deleteat!(Array(Any,5),5) both work fine.

Now that those sparse functions are using deleteat! instead of splice! the error is fixed, so the test just makes sure sparse matmul can still be used with abstract element types in the future. What did the error look like, something like no method zero(Any)?

@andreasnoack
Copy link
Member Author

I have added zero and one methods for Ptr. It shows that the definition in operators.jl was convenient because I have defined zero/one four times instead of the single definition in operators.jl. I don't like zero(Any)=0 and would prefer floating point for zero(Real) but the cons are about to outweigh the pros.

Another thing. I found out that zero(Ptr{None}) was needed when I started an IJulia notebook. Is it possible to have some tests for IJulia or is it not feasible?

@andreasnoack
Copy link
Member Author

@tkelman Thank you. Yes, it was a no method error for zero(Any).

zero(::Type{BigInt}) = BigInt(0)
zero(x::BigInt) = BigInt(0)
one(::Type{BigInt}) = BigInt(1)
one(x::BigInt) = BigInt(1)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Aren't these handled fine by the general case in number.jl?

@andreasnoack
Copy link
Member Author

No, because there is no convert(BigInt,Float64) method. It could be added, of course.

@StefanKarpinski
Copy link
Sponsor Member

I think adding the convert(BigInt,Float64) method would be preferable.

@JeffBezanson
Copy link
Sponsor Member

Done.

@andreasnoack
Copy link
Member Author

Thanks. Is it possible to define typemin/max(BigInt)? I think that would make the rational versions redundant.

@JeffBezanson
Copy link
Sponsor Member

Sadly no; BigInt has no infinities. How does typemin enter into this?

@andreasnoack
Copy link
Member Author

one(Rational{BigInt}) calls convert(Rational{BigInt},1.0) which calls rationalize(BigInt,1.0) and that function calls typemin.

…fine new zero and one methods for Rational and Ptr. Update tests.
@andreasnoack
Copy link
Member Author

I have now removed version for BigInt which are no longer needed with @JeffBezanson's update, but I have kept the specific versions of zero and one for rationals because rationalize(::Type{BigInt},FloatingPoint) doesn't work.

JeffBezanson added a commit that referenced this pull request Apr 4, 2014
Restrict zero and one to Number types. Default to floating points.
@JeffBezanson JeffBezanson merged commit bc1ea97 into master Apr 4, 2014
@andreasnoack andreasnoack deleted the anj/zeroone branch April 9, 2014 06:08
zero{T}(::Type{Ptr{T}}) = convert(Ptr{T}, 0)
zero{T}(x::Ptr{T}) = convert(Ptr{T}, 0)
one{T}(::Type{Ptr{T}}) = convert(Ptr{T}, 1)
one{T}(x::Ptr{T}) = convert(Ptr{T}, 1)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Why do we have one(Ptr{Int})? If I need a zero pointer I use C_NULL, but I can't see any reason to have a one pointer.

@andreasnoack
Copy link
Member Author

They were needed somewhere in Base, but I cannot remember anymore. Try commenting them out and see if they are still needed.

@ivarne
Copy link
Sponsor Member

ivarne commented Nov 5, 2014

All tests pass without both zero and one for Ptr{T}. I'll open a PR so that others will have some time to disagree before merging.

@jakebolewski
Copy link
Member

one is arguable but please don't get rid of zero(Ptr{T}), it is used in many packages outside of Base. I don't really see the value in removing either to be honest.

@ivarne
Copy link
Sponsor Member

ivarne commented Nov 5, 2014

Neither one nor zero for Ptr fulfills the documentation for one and zero about multiplicative and additive identity. Maybe I should fix the doc instead?

When is it applicable to use zero(Ptr{T}) instead of C_NULL?

@JeffBezanson
Copy link
Sponsor Member

I'd be curious to know why one was added. Either just for completeness, or something to do with multiplication and arithmetic, but I'm not sure what. It might be for ranges of pointers, left over from before Range had good support for ordinal types.

Easily getting a NULL pointer of a particular type seems useful though.

@ivarne
Copy link
Sponsor Member

ivarne commented Nov 5, 2014

But then we can't define zero as additive identity, because +(::Ptr{T}, ::Ptr{T}) isn't defined.

@jakebolewski
Copy link
Member

I guess now that Ptr{T}(0) works zero(Ptr{T}) is not that useful any more.

@andreasnoack
Copy link
Member Author

I only accidentally discovered that zero(Ptr{T}) was used when I restricted zero and one in this pr. Hence, I just added one to avoid breaking anything so I think it is safe to remove it.

@ivarne
Copy link
Sponsor Member

ivarne commented Nov 5, 2014

Ok, deprecations seems to be in order then.

ivarne added a commit to ivarne/julia that referenced this pull request Nov 5, 2014
They doesen't fullfill the documentation for zero and one about additive
and multiplicative identity.

See discussion in
JuliaLang#6183 (comment)
ivarne added a commit to ivarne/julia that referenced this pull request Nov 5, 2014
They doesn't fulfill the documentation for zero and one about additive
and multiplicative identity.

See discussion in
JuliaLang#6183 (comment)
ivarne added a commit to ivarne/julia that referenced this pull request Nov 5, 2014
They doesn't fulfill the documentation for zero and one about additive
and multiplicative identity.

See discussion in
JuliaLang#6183 (comment)
waTeim pushed a commit to waTeim/julia that referenced this pull request Nov 23, 2014
They doesn't fulfill the documentation for zero and one about additive
and multiplicative identity.

See discussion in
JuliaLang#6183 (comment)
@nalimilan nalimilan mentioned this pull request Jun 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants