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.

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

Jump to bottom

Add jaccard similarity metric #20

Merged
merged 12 commits into from
Aug 18, 2022
Merged

Add jaccard similarity metric #20

merged 12 commits into from
Aug 18, 2022

Conversation

tiagodavi
Copy link
Contributor

Calculate Jaccard similarity. I'll create a PR to calculate dissimilarity as well.

polvalente
polvalente reviewed Aug 17, 2022
View reviewed changes
@tiagodavi
Copy link
Contributor Author

@polvalente @jonatanklosko It's missing a case for binary attributes (the formula would be different for this case) .. What do you think of adding it as option?

type: :binary or something..

iex(1)> x = Nx.tensor [1,0,0,1,1,1]
#Nx.Tensor<
  s64[6]
  [1, 0, 0, 1, 1, 1]
>
iex(2)> y = Nx.tensor [0,0,1,1,1,0]
#Nx.Tensor<
  s64[6]
  [0, 0, 1, 1, 1, 0]
>
iex(3)> Scholar.Metrics.Similarity.jaccard(x, y)
#Nx.Tensor<
  f32
  1.0
>

It's was supposed to return 0.4 instead..

@polvalente
Copy link
Contributor

@tiagodavi I think adding a :type option is fine. What would be the correct formula for this case?

@jonatanklosko
Copy link
Member

The binary case has a different semantics and implementation, so I would have it separately as binary_jaccard. I believe this is similar to what we already do in Scholar.Metrics :)

@tiagodavi
Copy link
Contributor Author

It makes sense. Is there anything else missing to have this one approved?

@tiagodavi tiagodavi requested review from polvalente and jonatanklosko and removed request for polvalente and jonatanklosko August 17, 2022 21:58
@polvalente
Copy link
Contributor

I'll leave this open for a last pass on Jonatan's part

@jonatanklosko jonatanklosko changed the title add jaccard similarity Add jaccard similarity metric Aug 18, 2022
@jonatanklosko
Copy link
Member

Looks good, just a couple final comments!

tiagodavi and others added 2 commits August 18, 2022 09:26
@tiagodavi @jonatanklosko
Update test/scholar/metrics/similarity_test.exs
d731481 
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
@tiagodavi @jonatanklosko
Update lib/scholar/metrics/similarity.ex
ef0541d 
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
@tiagodavi
Copy link
Contributor Author

@jonatanklosko added

jonatanklosko
jonatanklosko approved these changes Aug 18, 2022
View reviewed changes
Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

:shipit:

@jonatanklosko jonatanklosko merged commit f0daae9 into elixir-nx:main Aug 18, 2022
@tiagodavi tiagodavi deleted the td/jaccard branch August 18, 2022 15:05
@tiagodavi
Copy link
Contributor Author

@polvalente @jonatanklosko Can I have my name Assigned to the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@polvalente polvalente polvalente approved these changes

@jonatanklosko jonatanklosko jonatanklosko approved these changes

Assignees

@tiagodavi tiagodavi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tiagodavi @polvalente @jonatanklosko