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

Fix the default implementation of Differentiable for Hashable #115

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

Conversation

yunhao
Copy link

@yunhao yunhao commented Sep 11, 2020

Checklist

Description

The default implementation of Differentiabe for Hashable has a differenceIdentifier property. The implementation of this propeprty now return hashValue instead of self.

Related Issue

// Model
struct ElementModel: Hashable, Differentiable {
    var id: Int
    var title: String
    var value: String

    func hash(into hasher: inout Hasher) {
        hasher.combine(id)
    }
}

// Changeset
let list1 = [ElementModel(id: 1, title: "color", value: "red")]
let list2 = [ElementModel(id: 1, title: "color", value: "blue")]

let changeset = StagedChangeset(source: list1, target: list2)
changeset.forEach { print($0) } // two stage: `delete` -> `insert`

The above code produces a delete -> insert change, however, it should be a update change.

#114: Unexpected behavior when using the default implementations of Differentiable for Hashable

Motivation and Context

Close #114

Impact on Existing Code

If the existing code rely on the default implementation of Differentiable for Hashable, the reload behavior may changes. Some delete -> insert changes will become update changes.

@guidedways
Copy link

guidedways commented Sep 30, 2021

Thanks for putting this in - I also think this is how it should work with Hashable. The whole point of differenceIdentifier is to uniquely identify an object. Returning self goes against the idea especially when both instances point to the same underlying object.

Would be nice to have this merged with the documentation reflecting the change.

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.

Unexpected behavior when using the default implementations of Differentiable for Hashable
3 participants