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

implement equality for ComposedFunction structurally #54877

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

Conversation

jw3126
Copy link
Contributor

@jw3126 jw3126 commented Jun 21, 2024

fix #53853

test/operators.jl Outdated Show resolved Hide resolved
@nsajko nsajko added the equality Issues relating to equality relations: ==, ===, isequal label Jun 21, 2024
@nsajko

This comment was marked as resolved.

@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> missingmissing
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

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.

@mikmoore
Copy link
Contributor

mikmoore commented Jun 24, 2024

I don't think it should be blocking here, and checking == for ComposedFunction seems weird and uncommon anyway, but do note that even the proposed definitions will say

julia> (sin  sin)  sin != sin  (sin  sin)
true

I.e, the binary nature of ComposedFunction's representation, contrasted with its strict right-to-left evaluation, means it is possible to construct semantically-equivalent ComposedFunction that differ only in their representation.

ComposedFunction constructed with the n-ary operator (i.e., ∘(sin, sin, sin)) will at least have consistent structure, but incremental constructions or cases where we force different associativity can have this discrepancy.


I do not think missing should be specially supported because it isn't callable. Any ComposedFunction call containing it will be a MethodError. If one constructs such a composition, it was almost-certainly by mistake.

@nsajko
Copy link
Contributor

nsajko commented Jun 24, 2024

possible to construct semantically-equivalent ComposedFunction that differ only in their representation

Interesting. Basically, is associative and we would ideally account for this symmetry in ==, because two ComposedFunction objects that differ only by associativity will always have the same result for the same inputs. Right?

One solution would be to recursively convert the ComposedFunction to Tuple, and then compare the two tuples of functions using ==(::Tuple, ::Tuple).

@mikmoore
Copy link
Contributor

Yes, you stated the issue much more clearly than I did. Possibilities would be to convert it to Tuple, canonicalize it, or to not convert them at all but simply traverse each in evaluation-order and compare their "elements" that way.

Although I think this PR would be fine even if it didn't recognize re-associated ComposedFunction as equivalent. That can be left for later. Personally, I don't remember the last time I checked == on a function (or even on a callable), much less a ComposedFunction. The likelihood that somebody assembles multiple versions with different associativity and then attempts to == them seems vanishing.

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.

@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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
equality Issues relating to equality relations: ==, ===, isequal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defining == for ComposedFunction by comparing inner and outer
4 participants