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

add functors SubFun, DivFun, and PowFun #12322

Merged
merged 3 commits into from
Jul 29, 2015
Merged

add functors SubFun, DivFun, and PowFun #12322

merged 3 commits into from
Jul 29, 2015

Conversation

lindahua
Copy link
Contributor

Several basic functors (subtraction, division, and power) are currently missing (which should be an oversight). This PR adds them.

These functors will be useful in the Sparse module. Since this is a very quick-fix, I make a quick PR for this, instead of putting these small changes to the SparseVector PR (which might take a relatively long time to get merged).

@tkelman
Copy link
Contributor

tkelman commented Jul 27, 2015

needs tests -

for op in (+, *, &, |)

@lindahua
Copy link
Contributor Author

tests added

@simonbyrne
Copy link
Contributor

One minor point: we really have three division functions: /, \ and div. So should DivFun really be RDivFun?

@lindahua
Copy link
Contributor Author

What about RDivFun, LDivFun and IDivFun? I can update the PR tomorrow if people think this is ok.

@jakebolewski
Copy link
Member

Those names seem better.

@lindahua
Copy link
Contributor Author

Since this is just a minor change. I will merge this tomorrow, if there's no objection.

@timholy
Copy link
Member

timholy commented Jul 28, 2015

I'm not sure if these two PRs affect each other in any way, but you might want to note the candidate introduction of GenericNFunc in #12292.

I suspect there will be some who would be happy about avoiding @generated functions, which this PR does. If we also add DotAddFun and friends, we might not even need GenericNFunc (at least, for those particular functions).

@lindahua
Copy link
Contributor Author

I think #12292 can be addressed using Func (without the need of introducing GenericNFunc). What's probably needed over that is some methods that accept input types and yield output type (such methods can be defined on sub-types of Func).

@timholy
Copy link
Member

timholy commented Jul 28, 2015

Yes, if we only want to cover a fixed list of operations, indeed we can just use manually-defined functors rather than going the parametric route.

@lindahua
Copy link
Contributor Author

From the discussion in #12357, it seems less controversial to just merge this and stay with this predefined list of functors. Then, we will wait for #10269 to land.

I will keep this open until tomorrow, and will merge if no objection.

@timholy
Copy link
Member

timholy commented Jul 29, 2015

Likewise, I'll wait on #12292 until this merges, and make use of it rather than introducing GenericNFunc.

We'll need versions of .* as well as *---do you want to add them here, or should I add them in #12292?

lindahua added a commit that referenced this pull request Jul 29, 2015
add functors SubFun, DivFun, and PowFun
@lindahua lindahua merged commit 9b14a7c into master Jul 29, 2015
@lindahua
Copy link
Contributor Author

You may add a functor for .* in #12292 if needed.

@tkelman tkelman deleted the dh/addfunctors branch July 29, 2015 21:26
@simonbyrne
Copy link
Contributor

Why not add a parametric Dot{F} functor?

@yuyichao
Copy link
Contributor

Why not add a parametric Dot{F} functor?

At least my objection for adding something parametric on a symbol defined in Base is that,

  1. We expect the function type overhaul to solve all these problems so adding these types is more or less a temporary solution.
  2. Defining these types are relatively easy to do so the user can define them even if base julia does not provide them
  3. Specialize on a symbol in base breaks the namespace. A good API should either be specific (like AddFun etc) or truly generic (e.g. it should work for user defined functions). Something in the middle will be really confusing.

@simonbyrne
Copy link
Contributor

I didn't mean for it to be symbol based: I meant use existing functors as parameters, eg Dot{AddFun}. But you're right in that I'm not sure in what it actually buys us.

@timholy
Copy link
Member

timholy commented Jul 31, 2015

I wonder whether @simonbyrne was asking whether .* should correspond to Dot{MulFun} rather than DotMulFun. See

julia/base/functors.jl

Lines 43 to 98 in db8b4ee

immutable DotAddFun <: Func{2} end
call(::DotAddFun, x, y) = x .+ y
immutable SubFun <: Func{2} end
call(::SubFun, x, y) = x - y
immutable DotSubFun <: Func{2} end
call(::DotSubFun, x, y) = x .- y
immutable MulFun <: Func{2} end
call(::MulFun, x, y) = x * y
immutable DotMulFun <: Func{2} end
call(::DotMulFun, x, y) = x .* y
immutable RDivFun <: Func{2} end
call(::RDivFun, x, y) = x / y
immutable DotRDivFun <: Func{2} end
call(::DotRDivFun, x, y) = x ./ y
immutable LDivFun <: Func{2} end
call(::LDivFun, x, y) = x \ y
immutable IDivFun <: Func{2} end
call(::IDivFun, x, y) = div(x, y)
immutable ModFun <: Func{2} end
call(::ModFun, x, y) = mod(x, y)
immutable RemFun <: Func{2} end
call(::RemFun, x, y) = rem(x, y)
immutable DotRemFun <: Func{2} end
call(::RemFun, x, y) = x .% y
immutable PowFun <: Func{2} end
call(::PowFun, x, y) = x ^ y
immutable MaxFun <: Func{2} end
call(::MaxFun, x, y) = scalarmax(x,y)
immutable MinFun <: Func{2} end
call(::MinFun, x, y) = scalarmin(x, y)
immutable LessFun <: Func{2} end
call(::LessFun, x, y) = x < y
immutable MoreFun <: Func{2} end
call(::MoreFun, x, y) = x > y
immutable DotLSFun <: Func{2} end
call(::DotLSFun, x, y) = x .<< y
immutable DotRSFun <: Func{2} end
call(::DotRSFun, x, y) = x .>> y
. But I'm not sure it would save much, unless you have a simple generic way of converting call(::MulFun, x, y) = x * y into call(::DotMulFun, x, y) = x .* y?

@yuyichao
Copy link
Contributor

I meant use existing functors as parameters, eg Dot{AddFun}.

This does sound more reasonable.

But I'm not sure it would save much, unless you have a simple generic way of converting call(::MulFun, x, y) = x * y into call(::DotMulFun, x, y) = x .* y?

But, yeah, this is why I didn't think this is what you meant...

@StefanKarpinski
Copy link
Member

Maybe somethign like this:

call{F<:ScalarFunctor}(Dot{F}, v::Vector, w::Vector) = [F(v[i],w[i]) for i=1:length(v)]

but, you know the actual general version (probably using broadcast).

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.

7 participants