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

Feature similarity groups #464

Merged
merged 11 commits into from
Nov 19, 2019
Merged

Feature similarity groups #464

merged 11 commits into from
Nov 19, 2019

Conversation

gusmith
Copy link
Contributor

@gusmith gusmith commented Nov 13, 2019

This is a first step to have the similarity score output for multi-party: this PR modifies the output from

[
  [row_index_0, row_index_1, score],
  ...
]

to

[
  {'group': [[party_id_0, row_index_0], [party_id_1, row_index_1]], 'sim': score},
  ...
]

which follows the groups output format of the candidate pairs (i.e. a record is represented by two indices [party_id, row_index].

Note that the tutorial does not work as long as the deployed entity-service is not updated with the current changes.
@gusmith gusmith requested a review from wilko77 November 13, 2019 06:10
And move some changelogs line to the next version instead of the last alpha release as they have not been integrated in it.
Copy link
Collaborator

@wilko77 wilko77 left a comment

Choose a reason for hiding this comment

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

I am concerned about the size of the output. By wrapping every sim score in a json object, we would almost double the amount of characters in the output.
I think we should go with something more streamlined. I made a suggestion in the comments, happy to discuss.

👍 for keeping the changelog up to date.

backend/entityservice/api_def/swagger.yaml Outdated Show resolved Hide resolved
backend/entityservice/api_def/swagger.yaml Outdated Show resolved Hide resolved
@gusmith gusmith requested a review from wilko77 November 15, 2019 05:52
Copy link
Collaborator

@wilko77 wilko77 left a comment

Choose a reason for hiding this comment

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

just some minor comments.
But have a look at the permutation notebook test. Something is not right.

backend/entityservice/api_def/swagger.yaml Outdated Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
@gusmith
Copy link
Contributor Author

gusmith commented Nov 18, 2019

The tutorials are currently faliing because of the newest release of clkhash which has some breaking changes.
The PR #465 is fixing them, but in progress.

@gusmith gusmith self-assigned this Nov 18, 2019
They may not work with the currently deployed service because of breaking changes.
@gusmith
Copy link
Contributor Author

gusmith commented Nov 19, 2019

@wilko77 Since your review, I simply merged dev into this branch (resolving some conflicts in a tutorial), and added a note about the tutorials usage, mainly because the ones from this branch are NOT working with the currently deployed service (because of some breaking change in the code which has not been deployed).

@gusmith gusmith requested a review from wilko77 November 19, 2019 05:41
@gusmith gusmith merged commit ab6f94f into develop Nov 19, 2019
@gusmith gusmith deleted the feature-similarity-groups branch November 19, 2019 06:22
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.

None yet

2 participants