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

changed isinteger, isreal, etc. #3071

Merged
merged 5 commits into from
May 14, 2013
Merged

Conversation

simonbyrne
Copy link
Contributor

From discussion of #3054

  • new isela iseltype function (I'm open to a better suggestion for the name)
  • deprecate isinteger, isreal, iscomplex, isbool
  • renamed integer_valued, real_valued, float64_valued to isintegervalued isinteger, etc. (I didn't change it to isinteger as that could cause problems with existing code/matlab & octave users).: this is a breaking change.

… iscomplex, isbool.

renamed integer_valued, real_valued, float64_valued
@StefanKarpinski
Copy link
Member

In general this is good although I'm unclear on why isela needs to exist – this is just <:.

@simonbyrne
Copy link
Contributor Author

It should be equivalent to

isela(x,T) = eltype(x) <: T

iscomplex seemed to be used quite a bit, so I thought it made sense to keep the similar function. However some of that usage should arguably be handled by dispatch, though some of it (such as the lapack) could be a bit tricky.

@pao
Copy link
Member

pao commented May 10, 2013

isela(x,T) = eltype(x) <: T is much easier to comprehend--is there a reason to use the definition you gave in your patch instead?

@simonbyrne
Copy link
Contributor Author

No, I only thought of the alternative after @StefanKarpinski's comment.

@JeffBezanson
Copy link
Member

Thanks for doing this!

I think iseltype would be better. And just writing out eltype(x) <: T is not so bad either.

I also think it is defensible for isinteger(3.0) to be true. The argument is, after all, an integer. isintegervalued is pretty verbose.

@StefanKarpinski
Copy link
Member

I would be cool with isinteger, isreal, et al. testing for value. But we could shorten them to isintval and isrealval. Since we consider numbers to be like single-element containers, it makes send that eltype of them would give their type. Again, I'm concerned about what eltype("foo") should be, but in practice it jet doesn't seem to be a big deal.

@JeffBezanson
Copy link
Member

I could go along with isintval, but it is always nice to use whole words when possible.

It is generally best for eltype of something iterable to give the type of the elements it generates.

@JeffBezanson
Copy link
Member

Looking over many function names, it seems that anything over 10 characters starts to seem long. Most times when a name struck me as "just barely ok" it had 10 characters (beginswith), and when one struck me as "starting to get long" it had 11 characters (ishermitian, permutedims). Seems subjective but I bet this is actually pretty robust.

@wlbksy
Copy link
Contributor

wlbksy commented May 11, 2013

how about isofinteger and isofreal

@StefanKarpinski
Copy link
Member

Doesn't really read very well. I think let's just go with isinteger and isreal and document it clearly.

@simonbyrne
Copy link
Contributor Author

Okay, I've made the changes. However I just had one other thought: another option would be something like

isval(x,Integer)
isval(x,Real)
isval(x,Float64)

Of course the problem would be how to handle arrays (currently no array methods are defined for isinteger (the new one), but they are for isinf).

@JeffBezanson
Copy link
Member

Yes we might need something like isconvertible(Int, x).

@JeffBezanson
Copy link
Member

I think the definitions of the form isinteger{T<:Integer}(::AbstractArray{T}) = true should remain, along with isinteger(a::AbstractArray) = all(isinteger, a). I know that's inconsistent with isinf, which is elementwise, but it seems right to me.

In general, it's best to use map(isinf, A), map(isinteger, A), all(isreal, A) etc., but adding the above definition allows isinteger to be O(1) on integer-typed arrays.

@simonbyrne
Copy link
Contributor Author

Okay, that should be all of the missing methods.

@JeffBezanson
Copy link
Member

I'd like to merge this. Ok with @StefanKarpinski @ViralBShah ?

@StefanKarpinski
Copy link
Member

Yeah, go for it.

@ViralBShah
Copy link
Member

LGTM.

JeffBezanson added a commit that referenced this pull request May 14, 2013
changed isinteger, isreal, etc.
@JeffBezanson JeffBezanson merged commit aa33b77 into JuliaLang:master May 14, 2013
@simonbyrne simonbyrne mentioned this pull request May 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants