Skip to content

implement equality for ComposedFunction structurally #54877

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

implement equality for ComposedFunction structurally #54877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ae4053
implement equality for ComposedFunction structurally
jw3126 Jun 21, 2024
2fd6d6a
Update test/operators.jl
jw3126 Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions base/operators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1094,6 +1094,19 @@ unwrap_composed(c) = (maybeconstructor(c),)
call_composed(fs, x, kw) = (@inline; fs[1](call_composed(tail(fs), x, kw)))
call_composed(fs::Tuple{Any}, x, kw) = fs[1](x...; kw...)

function hash(f::ComposedFunction, h::UInt)::UInt
h = hash(f.inner, h)
hash(f.outer, h)
end

function isequal(f1::ComposedFunction, f2::ComposedFunction)::Bool
isequal(f1.inner, f2.inner) && isequal(f1.outer, f2.outer)
end

function ==(f1::ComposedFunction, f2::ComposedFunction)::Bool
==(f1.inner, f2.inner) && ==(f1.outer, f2.outer)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
==(f1.inner, f2.inner) && ==(f1.outer, f2.outer)
==(f1.inner, f2.inner) & ==(f1.outer, f2.outer)

Support missing and other non-Bool results of == by not short-circuiting.

end

struct Constructor{F} <: Function end
(::Constructor{F})(args...; kw...) where {F} = (@inline; F(args...; kw...))
maybeconstructor(::Type{F}) where {F} = Constructor{F}()
Expand Down
31 changes: 31 additions & 0 deletions test/operators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,37 @@ end

end

@testset "ComposedFunction hash eq" begin
@test sin∘cos == sin∘cos
@test isequal(sin∘cos, sin∘cos)
@test !isequal(sin∘cos, sin∘asin)
@test ==(sin∘cos, sin∘cos)
@test !==(sin∘cos, sin∘asin)
@test hash(sin∘cos) == hash(sin∘cos)
@test hash(sin∘cos) != hash(sin∘tan)

Base.@kwdef mutable struct F
isequal::Bool = false
eq::Bool = false
end
function Base.isequal(f1::F, f2::F)
f1.isequal
end
function Base.:(==)(f1::F, f2::F)
f1.eq
end
f2 = F() ∘ F()
@test isequal(F(isequal=true) ∘ F(isequal=true) , f2)
@test !isequal(F(isequal=true) ∘ F(isequal=false) , f2)
@test !isequal(F(isequal=false) ∘ F(isequal=true) , f2)
@test !isequal(F(isequal=false) ∘ F(isequal=false) , f2)

@test ==(F(eq=true) ∘ F(eq=true) , f2)
@test !==(F(eq=true) ∘ F(eq=false) , f2)
@test !==(F(eq=false) ∘ F(eq=true) , f2)
@test !==(F(eq=false) ∘ F(eq=false) , f2)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
end
@test ((missing ∘ sin) == (missing ∘ sin)) === missing
@test ((missing ∘ sin) == (missing ∘ cos)) === false
end

Not sure, but presumably we'd like to intepret ComposedFunction like a collection of functions, thus suppport missing as usual.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a good point, but orthogonal to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, right now missing is not handled specially by composion.

julia> missing∘missing
missing ∘ missing

Copy link
Contributor

Choose a reason for hiding this comment

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

orthogonal to this PR.

I don't think so. Right now we have the excuse that == for ComposedFunction isn't implemented at all, but if we're finally implementing it presumably the handling of missing should be thought through.

right now missing is not handled specially by composion.

Composition doesn't need to handle missing specially, just ==. If ComposedFunction is interpreted as a sort of collection of functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, sorry I misread some parenthesis in your test. I read (missing ∘ sin) === missing.

Copy link
Contributor Author

@jw3126 jw3126 Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My takeaway from #54881 is ot not implement special logic for missing in composed function equality in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's possible to support missing without any special missing-specific logic.


@testset "Nested ComposedFunction's stability" begin
f(x) = (1, 1, x...)
g = (f ∘ (f ∘ f)) ∘ (f ∘ f ∘ f)
Expand Down