-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 one arg function composition #34251
Conversation
base/operators.jl
Outdated
@@ -856,6 +859,8 @@ julia> ∘(fs...)(3) | |||
3.0 | |||
``` | |||
""" | |||
∘() = identity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is the best way. For example, ∘(f, g, ∘())
may not work out-of-the-box if f
and g
are generic morphisms like lenses or transducers. It's the same reason why we don't have +()
or *()
. OTOH, it's nice that we have a canonical identity for ∘
. I guess I'd be less worried if
∘(f, ::typeof(identity)) = f
∘(::typeof(identity), f) = f
∘(::typeof(identity), ::typeof(identity)) = identity
are implemented as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a very good observation. My favored solution would be dropping ∘()
and adding your remark as a comment I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps ∘() = error(your remark)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's based on the desire to have ∘
for other "morphisms" than functions, which has been requested before, e.g. in the discussion where compose
was requested as an ASCII name for ∘
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding only ∘(f)
sounds good. Adding things is much easier than removing things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also be fine with just adding the definitions you proposed:
∘(f, ::typeof(identity)) = f
∘(::typeof(identity), f) = f
∘(::typeof(identity), ::typeof(identity)) = identity
So I guess we have two options: less or more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'm also fine with both choices.
I actually like adding that three-line definition with ::typeof(identity)
. It makes writing morphisms slightly easier. But this can happen independently of ∘()
. If we need some excuse for adding that three-line definition, and if ∘()
is enough for that, then I'd vote for adding ∘()
. Yeah, that's a bit 😈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I can live with both choices, here are some arguments why less is more:
I take lens composition as an example, but this applies to transducers and probably most kinds of morphisms.
- When I write
lens = ∘(lenses...)
I expect that I can use the lens api on the result. Likeget(obj, lens)
. This is impossible to achieve for the empty case. Now defining∘() = identity
(along with @tkf clever extra methods say) means that some part of the lens will work e.g∘(lens, ∘())
and others will not.
I would favor if everything fails at composition time with a sane error and not having some stuff work and other stuff throw a latent error about using lens api onidentity
. - Morphisms often ship their own identity (since you want an identity of type
::Lens
). In order to make things work, one must also define e.g.
∘(::IdenityLens, typeof(∘()))
∘(typeof(∘()), ::IdenityLens)
Not the end of the world, but having two identities is an extra stumbling block package authors need to keep in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are very good points. I'm now against ∘()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so let's go with the less option then: remove the ∘()
method and fix up the docs to correspond to that and this should be good to go.
base/operators.jl
Outdated
# While `∘() = identity` is a reasonable definition for functions, this | ||
# would cause headaches for composition of user defined morphisms. | ||
# See also #34251 | ||
msg = """Empty composition ∘() is undefined.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why define it at all then? Why not just let it be a method error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that we don't get a stream of PRs that want to define it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to leave it undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the benefit of making it very clear that not defining it is intentional with this approach. But I think leaving a comment is enough to prevent future PRs.
There seems to be trailing whitespace. |
Thanks for sticking this process out! Who knew this would require so much discussion? |
Is it possible to have Right now it can be done recursively: h(x) = x^2 - x + 1
hh(x, n) = (n == 0) ? x : (h∘hh)(x, n-1)
h(2), h(h(2)), h(h(h(2)))
(h)(2), (h∘h)(2), (h∘h∘h)(2)
hh(2,1), hh(2,2), hh(2,3)
hh(2,4) While we're here can we return to multivariate function composition? |
@azev77 thats a nice idea. There are some good arguments against defining |
can we correcrt the name of this PR? |
Thinking about it some more, I am not sure I like the name anymore. (∘^2) = (∘)∘(∘)
(∘^3) = (∘)∘(∘)∘(∘)
... would be another plausible meaning for |
@jw3126 there might be other options, but we need something for function iteration. In Mathematica: Given a function h(x) = x^2 - x + 1
h^2(x) = h(h(x))
h^n(x)=(h∘ h∘...∘h)(x) |
I’m ok with |
I see your point, |
Yes, that's why I'm somewhat inclined to define it for function iteration. |
I think defining |
One notation I occasionally see and like for iterating operations other then x^⊗3 = x ⊗ x ⊗ x
f^∘2 = f ∘ f I don't think its worth to change the parser for this, but maybe we could have ^(op, x, n) = ...
^(x, n) = ^(*, x, n)
^(∘, f, 3) = f ∘ f ∘ f # or some variant type stable in n In theory one could also overload |
I started to wonder if In this scenario, f^(n::Integer) = x0 -> foldl((x, _) -> f(x), 1:n; init = x0) If |
See #33568 (comment)