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

Stripe identity off ComposedOptic when test equality #94

Closed
wants to merge 2 commits into from

Conversation

sunxd3
Copy link
Member

@sunxd3 sunxd3 commented Apr 8, 2024

The method

function Base.:(vn::VarName{sym,<:Lens}, lens::Lens) where {sym}
    return VarName{sym}(getlens(vn)  lens)
end

was removed with the #91.

The trigger was: Accessors changes the order of composition (Accessors follows function composition order, i.e., f ∘ g mean apply g first, which I think is more intuitive), so we needed to adjust the above function for the new order or remove it.
With my personal taste, I removed it (sorry for missing @torfjelde's comment here), because it strikes me a bit strange composing a VarName and an optic.

WIthout this function, In DynamicPPL, we will need to do something like VarName{getsym(vn)}(optic ∘ getoptic(vn)) explicitly, which works okay (modulo the extra code). (One current example in DynamicPPL using between VarName and Lens.)

But one issue surface:
Sometimes in DynamicPPL we need to retrieve a value from a dictionary like Dict(VarName{:a}(IndexLens(1)) => 1.0, ...), with varname VarName{:a}(IndexLens(1) ∘ identity). Just as Tor's concern, this will fail.

This PR implements a more allowing equality tests to circumvent this issue. But I am also happy to consider restoring the aforementioned Base.:∘(vn::VarName{sym,<:Lens}, lens::Lens) function.

@sunxd3
Copy link
Member Author

sunxd3 commented Apr 8, 2024

@tor @devmotion @yebai opinions?

@yebai
Copy link
Member

yebai commented Apr 8, 2024

Probably better for @torfjelde to review this?

@yebai yebai requested a review from torfjelde April 8, 2024 15:21
@sunxd3
Copy link
Member Author

sunxd3 commented Apr 9, 2024

PR closed as this seems to be a dead end -- ComposedOptic has Inner and Outer type in the signature, so the Dict key type will never match even with == defined.

@sunxd3 sunxd3 closed this Apr 9, 2024
@sunxd3 sunxd3 deleted the sunxd/equality_with_identity_optic branch April 9, 2024 15:05
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.

2 participants