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

Function composition and negation, take 2 #17184

Closed
wants to merge 7 commits into from

Conversation

andyferris
Copy link
Member

Something strange happened to my previous PR #17119, and github closed it.

This is the basic version with just an anonymous function version of and a definition of function negation. This is quite related to #17155.

@StefanKarpinski feel free to just take docstrings and tests (in general - is it better to make a PR to your PR?)

|>(x, f) = f(x)

"""
∘(f, g)
f ∘ g
Copy link
Contributor

Choose a reason for hiding this comment

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

would need to put the signatures into the RST docs so make docs populates the docstrings there, see details in contributing.md

Copy link
Member Author

Choose a reason for hiding this comment

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

I added .. function:: ∘(f, g) to base.rst and, although it worked for .. function:: !(f::Function), I get the following:

WARNING: missing docs for signature:

    ∘(f, g)

Is this something to do with unicode operators?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tkelman (not sure if you'd get auto-pinged or not, here)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, no need to explicitly ping me if I've already commented on this issue. It may be a unicode issue where you might have to explicitly declare unicode operators somewhere for RST/Sphinx (I'm not sure where that is though), or it may have to do with the separate signatures being on multiple lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, yes it is the latter

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a little unfortunate though. Infix operators should allow an infix form. Even typing ?<: in the REPL gives help for <:(T1, T2) - have you ever seen a single line of Julia code written like that??

Copy link
Member

Choose a reason for hiding this comment

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

At least it shows you that operators are just standard functions (OK, not true for <:) and that you can add a method using the standard syntax. But we should definitely improve these docstrings by adding the infix forms on the second line at the minimum.

Copy link
Member

Choose a reason for hiding this comment

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

@andyferris did you include the entire signature, i.e.

.. function:: ∘(f, g)
              f ∘ g

or just the first line .. function:: ∘(f, g)? Signature matching requires the entire first code block, not just the first line of it.

@TotalVerb
Copy link
Contributor

TotalVerb commented Jun 29, 2016

How is performance? Does the splatting penalty apply here?

Edit: Yikes, it's not good at all. This definition helps a lot:

function ∘(f, g)
    h(x) = f(g(x))
    h(xs...) = f(g(xs...))
    h
end

however it probably slows down itself.

@andyferris
Copy link
Member Author

andyferris commented Jun 29, 2016

@TotalVerb Good question! Simple check says "no""yes" - splatting penalty is applying. @StefanKarpinski do you know where we are up to here? I have seen some issues that discuss this and I thought there was a simple fix in place for f(x...) = g(x...) patterns (although this is epsilon more complicated than that).

@andyferris
Copy link
Member Author

Indeed, if we can't work around it, then maybe we have to follow @TotalVerb's suggestion until the splatting is fixed. I would suggest generating versions up to MAX_TUPLE_SPLAT (currently 16), for consistency.

@andyferris
Copy link
Member Author

however it probably slows down ∘ itself.

A functor approach might help with that... define 17 methods globally on calling a ComposedFunction{F,G} and not generate them every time someone calls .

@TotalVerb
Copy link
Contributor

TotalVerb commented Jun 29, 2016

I don't like calling these "functors", especially since (from the user's perspective) they're not meaningfully different from plain old closures.

I guess you mean something like this? I did not get a chance to look at your last PR, but I assume it was a similar approach?

immutable ComposedFunction{F,G} <: Function
    f::F
    g::G
end

This is basically the same as the autogenerated closure type, but more flexible I suppose.

@TotalVerb
Copy link
Contributor

TotalVerb commented Jun 29, 2016

Actually, since h is a closure, the methods should already be added only once—not once on each call—so the functor should be unnecessary. Indeed, checking code_native one observes that is compiled into a no-op.

@andyferris
Copy link
Member Author

OK, yes, functor is a strange name. I meant exactly the thing your wrote, ComposedFunction.

I agree the h as a closure will work fast and "only be constructed once per composed function". I was suggesting that we only define the 17 splatting methods "once per session of Julia".

I actually don't care if there is a once-off overhead to . Just one more reason to fix the splatting penalty!

@TotalVerb
Copy link
Contributor

@andyferris What I meant was that my assessment was incorrect, and there is no such cost when creating a composed function, because methods are on the (abstract) closure type. So the performance of a "functor" is equivalent to the performance of a closure.

Decided to make up to 16 inputs fast, to match MAX_TUPLE_SPLAT.
Conveniently, closures also allow for introspection of f and g.
@andyferris
Copy link
Member Author

Hmm... its a bit uglier, but we now have no splatting penalty!

I just discovered closures let us have access to the two things we are composing, allowing for introspection. Cool!

(Closures also seem to be a type of "generated type", in the sense the field names and number depend on how you define them. Super cool!)

"""
!(f::Function) = (x...)->!f(x...)
function !(f::Function)
Copy link
Member

Choose a reason for hiding this comment

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

Can't you just define this as ! ∘ f to avoid repeating the same machinery as above?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to, and I tried that first, but I got some kind of recursion when I tried to compile Julia. I didn't have time to investigate the cause of that yet, so I just used copy-paste.

Copy link
Contributor

@TotalVerb TotalVerb Jun 30, 2016

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's because ! ∘ f doesn't parse correctly. We probably need (!) ∘ f. This is a little ugly, which makes me like !f syntax a little more.

Copy link
Member Author

@andyferris andyferris Jun 30, 2016

Choose a reason for hiding this comment

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

That worked... thanks!

@andyferris
Copy link
Member Author

andyferris commented Jun 30, 2016

I'm pretty sure that's because ! ∘ f doesn't parse correctly. We probably need (!) ∘ f. This is a little ugly, which makes me like !f syntax a little more.

We will probably see this with other operators. This is an argument for even more overloads of operators on functions, as in #17155... it would even suggest doing it for every operator. Not sure if I like that or not though!

EDIT: or at least every unary operator, since composition is more complex for binary.

@@ -55,3 +55,6 @@ let xs = [[i:i+4;] for i in 1:10]
@test max(xs[1:n]...) == [n:n+4;]
end
end

@test ((x -> x+1) ∘ (x _> 2x))(5) == 11
Copy link
Contributor

Choose a reason for hiding this comment

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

typo @test ((x -> x+1) ∘ (x -> 2x))(5) == 11

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks

@andyferris
Copy link
Member Author

@StefanKarpinski @JeffBezanson Where are we on this, and what is the probability of 0.5 inclusion?

Between here and #17155, the seems pretty well received. There are some contentions with unary operators like ! and much, much stronger contentions with binary operators like &, on the basis of not knowing where to draw the line on auto-composition.

We could maybe try ! as an experiment in 0.5? Or drop it completely?

"""
∘(f, g)

Creates a composition of two functions (or functor objects), such that
Copy link
Contributor

Choose a reason for hiding this comment

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

this pr doesn't include functor objects any more, does it?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it uses closures. But ∘ accepts functors (any callable) as arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll edit it to make it clear.

@oxinabox
Copy link
Contributor

oxinabox commented Aug 29, 2016

I noted today a place I wanted to used this: Eager-ifying lazy operations

function lazyload_semcor(tagdir_path::AbstractString)
    Task(...)
end

load_semcor(tagdir_path::AbstractString) = collect(lazyload_semcor(tagdir_path))

This would become:

load_semcor = collect∘lazyload_semcor

Which is nice.

@andyferris
Copy link
Member Author

Yes, that's right. I quite enjoy using these already in our CoordinateTransformations for similar reasons. You gain a lot of control on how things are done with little user effort.

The reason my original PR had a ComposedFunction object was to allow for even more in-depth introspection where composing certain things together might do something non-trivial - an example of the top of my head is map ∘ reduce -> mapreduce (because they take multiple arguments that doesn't work as I wrote it, obviously, but you can see we can skip an allocation and go straight to iteration - in other words going the opposite way you said, from eager to lazy, but only where appropriate).

@oxinabox oxinabox mentioned this pull request Oct 1, 2016
@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2016

closing in favor of #17155

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.

8 participants