Skip to content

Commit

Permalink
No more ambiguity warnings.
Browse files Browse the repository at this point in the history 
Also renamed svd.jl to linalg.jl in preparation for DataMatrix type.
  • Loading branch information
@johnmyleswhite
johnmyleswhite committed Dec 3, 2012
1 parent f8282e8 commit 5effea5
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 12 deletions.
6 changes: 2 additions & 4 deletions src/datavec.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,6 @@ end
length(x::NAtype) = 1
size(x::NAtype) = ()

==(na::NAtype, na2::NAtype) = NA
==(na::NAtype, b) = NA
==(a, na::NAtype) = NA

# constructor from type
function _dv_most_generic_type(vals)
# iterate over vals tuple to find the most generic non-NA type
Expand Down Expand Up @@ -505,6 +501,7 @@ assign{T}(x::PooledDataVec{T}, n::NAtype, rng::Range1) = begin (x.refs[rng] = 0)

# TODO: replace!(x::PooledDataVec{T}, from::T, to::T)
# and similar to and from NA
replace!{R}(x::PooledDataVec{R}, fromval::NAtype, toval::NAtype) = NA # no-op to deal with warning
function replace!{R, S, T}(x::PooledDataVec{R}, fromval::S, toval::T)
# throw error if fromval isn't in the pool
fromidx = findfirst(x.pool, fromval)
Expand Down Expand Up @@ -649,6 +646,7 @@ function done(x::AbstractDataVec, state::Int)
end

# can promote in theory based on data type
promote_rule{T, T}(::Type{AbstractDataVec{T}}, ::Type{T}) = promote_rule(T, T)
promote_rule{S, T}(::Type{AbstractDataVec{S}}, ::Type{T}) = promote_rule(S, T)
promote_rule{T}(::Type{AbstractDataVec{T}}, ::Type{T}) = T
# TODO: Abstract? Pooled?
Expand Down
0 src/svd.jl → src/linalg.jl
View file
File renamed without changes.
35 changes: 30 additions & 5 deletions src/operators.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,26 @@ scalar_comparison_operators = [:(==), :(!=), :isless, :(>), :(>=),
array_comparison_operators = [:(.==), :(.!=), :(.>), :(.>=), :(.<), :(.<=)]

binary_operators = [:(+), :(.+), :(-), :(.-), :(*), :(.*), :(/), :(./),
:(^), :(.^), :(div), :(mod), :(fld), :(rem),
:(div), :(mod), :(fld), :(rem),
:(&), :(|), :($)]

induced_binary_operators = [:(^), :(.^)]

arithmetic_operators = [:(+), :(.+), :(-), :(.-), :(*), :(.*), :(/), :(./),
:(^), :(.^), :(div), :(mod), :(fld), :(rem)]
:(div), :(mod), :(fld), :(rem)]

induced_arithmetic_operators = [:(^), :(.^)]

biscalar_operators = [:(max), :(min)]

scalar_arithmetic_operators = [:(+), :(-), :(*), :(/), :(^),
scalar_arithmetic_operators = [:(+), :(-), :(*), :(/),
:(div), :(mod), :(fld), :(rem)]

array_arithmetic_operators = [:(+), :(.+), :(-), :(.-), :(.*), :(./), :(.^)]
induced_scalar_arithmetic_operators = [:(^)]

array_arithmetic_operators = [:(+), :(.+), :(-), :(.-), :(.*), :(./)]

induced_array_arithmetic_operators = [:(.^)]

bit_operators = [:(&), :(|), :($)]

Expand All @@ -53,7 +61,7 @@ columnar_operators = [:colmins, :colmaxs, :colprods, :colsums,

boolean_operators = [:any, :all]

# ^ is a special case as it's defined in terms of *
# TODO :(^) is a special case as it's defined in terms of :(*)

for f in unary_operators
@eval begin
Expand Down Expand Up @@ -310,6 +318,23 @@ for f in binary_operators
function ($f){T <: Union(String, Number)}(x::T, d::NAtype)
return NA
end
function ($f){T,N}(a::AbstractArray{T,N}, d::NAtype)
return NA
end
function ($f){T,N}(d::NAtype, a::AbstractArray{T,N})
return NA
end
end
end

for f in induced_binary_operators
@eval begin
function ($f)(d::NAtype, e::NAtype)
return NA
end
function ($f)(d::Union(String, Number), e::NAtype)
return NA
end
end
end

Expand Down
21 changes: 18 additions & 3 deletions test/operators.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,27 @@ scalar_comparison_operators = [:(==), :(!=), :isless, :(>), :(>=),

array_comparison_operators = [:(.==), :(.!=), :(.>), :(.>=), :(.<), :(.<=)]

binary_operators = [:(+), :(.+), :(-), :(.-), :(*), :(.*), :(/), :(./),
:(div), :(mod), :(fld), :(rem),
:(&), :(|), :($)]

induced_binary_operators = [:(^), :(.^)]

arithmetic_operators = [:(+), :(.+), :(-), :(.-), :(*), :(.*), :(/), :(./),
:(^), :(.^), :(div), :(mod), :(fld), :(rem)]
:(div), :(mod), :(fld), :(rem)]

induced_arithmetic_operators = [:(^), :(.^)]

biscalar_operators = [:(max), :(min)]

scalar_arithmetic_operators = [:(+), :(-), :(*), :(/), :(^),
scalar_arithmetic_operators = [:(+), :(-), :(*), :(/),
:(div), :(mod), :(fld), :(rem)]

array_arithmetic_operators = [:(+), :(.+), :(-), :(.-), :(.*), :(./), :(.^)]
induced_scalar_arithmetic_operators = [:(^)]

array_arithmetic_operators = [:(+), :(.+), :(-), :(.-), :(.*), :(./)]

induced_array_arithmetic_operators = [:(.^)]

bit_operators = [:(&), :(|), :($)]

Expand Down Expand Up @@ -413,3 +425,6 @@ df = DataFrame(quote
df[1, 1] = NA
@assert isna(any(dv))
@assert isna(all(dv))

# TODO: Confirm that this is not true any longer:
# no method .^(NAtype,Int64)

7 comments on commit 5effea5

@johnmyleswhite
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commit fixes all of the ambiguity warnings generated by the DataFrames package.

There is still one warning about an ignored attempt to redefine NARule. As far as I can see, someone tried to use different capitalizations of NARule to mean different things. I'll resolve that at some point, although possibly by removing the entire field naRule from DataVec's.

@tshort
Copy link
Contributor

@tshort tshort commented on 5effea5 Dec 3, 2012

Choose a reason for hiding this comment

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

!!!!! Great, John !!!!!

@johnmyleswhite
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am pretty happy about this as well.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is great, John. Question: do you feel like this was an annoying and somewhat pointless hassle, or did the ambiguity warnings serve a purpose? I.e. did this make the code better? Also, those classifications of operators are rather useful and should maybe be made more generally available for metaprogramming purposes.

cc: @JeffBezanson. This relates to the "operator dispatch" idea I told you about the other day. These sets of operators would presumably correspond to operator types and you would add methods apply for the corresponding operator types instead of using @eval to loop over all the individual operators. We would have to get the operator classification hierarchy correct, but I think that's doable.

@johnmyleswhite
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For NA's, the ambiguity warnings were extremely valuable in retrospect, since they indicated places where NA's were poisoning things inappropriately. They're frustrating at the time of creation, but they did at least make it clear how to get NA's to work correctly.

I agree that whatever metaprogramming module we eventually develop should contain those kind of classifications of operators. I was just starting to write a test_type() function that would do something similar for new types: add all basic operators and confirm that things like print, show, and repl_show have explicit definitions.

I'd like to hear more about the operator dispatch idea.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

The basic idea for operator dispatch – and this is a possibly whackadoodle, bad idea, but it seems interesting – is that you would have a type classification on functions and could, e.g., declare that -::UnaryOperator (somehow, totally unclear how, maybe - = UnaryOperator() to create a new generic function of type UnaryOperator?). Then later, instead of doing this:

for f in unary_operators
    @eval ($f)(::NAtype) = NA
end

you could write this instead:

apply(f::UnaryOperator,::NAType) = NA

In the absence of a specific definition of -(::NAType), this dispatch rule would get invoked when -NA occurred, and you'd get the desired behavior.

This idea has a few effects and consequences. In some way that we haven't really fully thought through yet, it unifies types and functions. This unification may or may not be compatible with the way that some types can currently be invoked as constructors. It also makes function syntax overloadable, which is Jeff's biggest qualm about it – it makes the worst case performance of method dispatch potentially much, much worse, without significant cleverness. Finally, it seems like it would require multiple inheritance to make it fully useful since most operators would need to be in multiple overlapping classes of operators. Since multiple inheritance is something we're not considering until a much later version of Julia because of implementation complexity, this seems like it would relegate operator dispatch to at least that far into the future. But considering it now is not a bad idea since we might be able to leave room for it.

@johnmyleswhite
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been thinking more about operator dispatch. I still think the idea is awesome, but keep finding myself needing more and more complex multiple inheritance-like constructs to reason about how operators work (e.g. my recent work on matrix multiplication for DataMatrix). Enumerating some basic properties and making them programmatic in any way still seems valuable. For instance, I've come to think that defining a @commutative macro is a little tricky, but that having a is_commutative test is a clear win for reasoning about operators.

Please sign in to comment.