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

Expose ComposedFunction as a public API #37517

Merged
merged 15 commits into from
Sep 24, 2020
Merged

Conversation

jw3126
Copy link
Contributor

@jw3126 jw3126 commented Sep 10, 2020

cc @tkf

julia> composition = sin ∘ cos
sin ∘ cos

julia> composition.f === sin
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 would actually prefer to name the fields to e.g. outer, inner instead of f,g. It is hard to remember if this models f∘g or g∘f.

Copy link
Member

Choose a reason for hiding this comment

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

I'm somewhat neutral about the choice of the field names. I see f and g as something like x and y. It's technically arbitrary but I've seen it many times so I don't think it's super hard to guess the ordering.

Just as a note, one (rather weak) upside of keep using f and g is that we can define ComposedFunction for older julia versions as done in DataFrames.jl (JuliaData/DataFrames.jl#2274 (comment)). However, it doesn't matter much since 1.6 soon will be the LTS (and we can even define getproperty for the closure in Compat.jl).

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 think the order is not easy to guess at all. Just have a look at the wikipedia article. There the order g∘f is more dominant, which is opposed to the current order we use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My personal first guess was btw g∘f.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I sometimes get inner/outer opposite somehow (though this is probably just me). Since you can always flip the arrows (and which direction feels natural depends on the context), I'm not sure if there are truly unambiguous names for them. But I'm likely nitpicking. I agree inner/outer is more descriptive for function composition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With outer, inner it is at least clear that c::ComposedFunction talks about c.outer(c.inner(x)). With f,g it is unclear whether we are talking about c.f(c.g(x)) or c.g(c.f(x)). Remembering the direction of arrows is an independent problem that comes up in both scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, I also get inner outer wrong sometimes, but I can usually recover by pure reasoning without having to dig up the source.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it's easier to remember and be confident which function is applied first with inner/outer. I think I'm in the team inner/outer now. How about renaming them in this PR and see the reactions of other reviewers?

Copy link
Contributor

@cmcaine cmcaine Sep 17, 2020

Choose a reason for hiding this comment

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

For the little it is worth, I think f and g are fine and their exact interpretation can be clarified in the docstring. That way there doesn't need to be any work on Compat.jl or in other packages.

If you stick with inner and outer, then I think line 881 needs to change:

Represents the composition of two callable objects `f::Outer` and `g::Inner`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, f and g are confusing, we should not force people to look up the docstring all the time. Also @tkf has a PR to compat
JuliaLang/Compat.jl#720 and supporting f and g is the easy part of that code.

base/operators.jl Outdated Show resolved Hide resolved
Co-authored-by: Jonas Schulze <[email protected]>
base/operators.jl Outdated Show resolved Hide resolved
Co-authored-by: Jonas Schulze <[email protected]>
@tkf
Copy link
Member

tkf commented Sep 10, 2020

@jw3126 Thanks for opening the PR!

For the record, here are some motivating examples:

In all these examples, we need a way to dispatch on ComposedFunction and access its fields. So, I think promoting ComposedFunction including its fields as a public API is a good idea.

@bkamins @nalimilan I think you guys want this to be public API too so that DataFrames.jl don't rely on the internal details of Base?

base/operators.jl Outdated Show resolved Hide resolved
base/operators.jl Outdated Show resolved Hide resolved
@tkf tkf changed the title document ComposedFunction Expose ComposedFunction as a public API Sep 10, 2020
@tkf tkf added the needs news A NEWS entry is required for this change label Sep 10, 2020
@tkf
Copy link
Member

tkf commented Sep 10, 2020

Since we are "adding" new API, I think it makes sense to put it in NEWS.

@tkf tkf removed the needs news A NEWS entry is required for this change label Sep 10, 2020
base/operators.jl Outdated Show resolved Hide resolved
@tkf
Copy link
Member

tkf commented Sep 10, 2020

I wrote JuliaLang/Compat.jl#720 so that we can use the same interface (ComposedFunction and outer/inner fields) in all Julia 1.x.

@tkf tkf requested a review from nalimilan September 10, 2020 22:57
@tkf
Copy link
Member

tkf commented Sep 10, 2020

@nalimilan Do you mind reviewing this, as I think DataFrames.jl is the biggest package affected by this patch (esp. the new field names; see discussion above #37517 (comment))? I think JuliaLang/Compat.jl#720 can minimize the effect, though.

@JeffBezanson
Copy link
Member

If we really want to make this official, might as well export it?

@JeffBezanson
Copy link
Member

Looks like there is a small doctest error to fix, otherwise LGTM.

@jw3126
Copy link
Contributor Author

jw3126 commented Sep 23, 2020

I think freebsd fail is unrelated?

@tkf
Copy link
Member

tkf commented Sep 23, 2020

I requested re-running it yesterday https://build.julialang.org/#/buildrequests/88695 but there is a long queue for freebsd buildbot https://build.julialang.org/#/builders/33

But yeah, it looks like it's unrelated.

@tkf
Copy link
Member

tkf commented Sep 24, 2020

So, it looks like I restarted two freebsd CIs

The first once passed: https://build.julialang.org/#/builders/33/builds/3979

The second one failed: https://build.julialang.org/#/builders/33/builds/3982


See also [`∘`](@ref).
"""
struct ComposedFunction{O,I} <: Function
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, This is coming very late.

Just wanted to find out if it is necessary to restrict O and I to be subtypes of Base.Callable as in

struct ComposedFunction{O<:Base.Callable, I<:Base.Callable} <: Function

Copy link
Member

Choose a reason for hiding this comment

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

There is no reason to restrict to Callable here. There are lots of callable objects that aren't a subtype of Callable and this would break those cases.

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