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 table normalization #3

Merged
merged 14 commits into from
Oct 2, 2021
Merged

Implement table normalization #3

merged 14 commits into from
Oct 2, 2021

Conversation

mrr00b00t
Copy link
Contributor

Also include tests for it.

src/TableDistances.jl Outdated Show resolved Hide resolved
src/TableDistances.jl Outdated Show resolved Hide resolved
src/TableDistances.jl Outdated Show resolved Hide resolved
src/TableDistances.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

You should use different tables in the test set instead of modifying an existing test set that is working. Can you please copy the tables to a separate section in the test suite? That way we make sure that the tables that are being tested are not already normalized in the distance calculations.

mrr00b00t and others added 5 commits October 2, 2021 12:07
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
@mrr00b00t mrr00b00t requested a review from juliohm October 2, 2021 15:22
src/TableDistances.jl Outdated Show resolved Hide resolved
src/TableDistances.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

Just a minor change now.

@mrr00b00t mrr00b00t requested a review from juliohm October 2, 2021 15:56
test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

Please create a testset for the pairwise like I did for the Normalization. These are independent test sets.

@mrr00b00t mrr00b00t requested a review from juliohm October 2, 2021 16:10
@juliohm
Copy link
Member

juliohm commented Oct 2, 2021

Tests are failing, can you run locally and check that?

src/TableDistances.jl Outdated Show resolved Hide resolved
@juliohm juliohm merged commit 86125ed into master Oct 2, 2021
@juliohm juliohm deleted the table-normalization branch October 2, 2021 16:38
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