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

FDVerifier for several rhs columns #236

Merged
merged 3 commits into from
Aug 8, 2023

Conversation

MichaelS239
Copy link
Collaborator

Change the existing FD verification algorithm to cover the case when there are several columns on the right hand side

tests/test_fd_verifier.cpp Show resolved Hide resolved
src/algorithms/fd_verifier/fd_verifier.cpp Outdated Show resolved Hide resolved
src/algorithms/fd_verifier/stats_calculator.cpp Outdated Show resolved Hide resolved
Comment on lines 68 to 70
std::unordered_map<ClusterIndex, unsigned> frequencies =
util::PLI::CreateFrequencies(cluster, pt);
size_t num_distinct_rhs_values = CalculateNumDistinctRhsValues(frequencies, cluster.size());
Copy link
Collaborator

@polyntsov polyntsov Jul 31, 2023

Choose a reason for hiding this comment

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

я сейчас перечитывал этот код и понял, что эта логика на самом деле выглядит довольно плохо. Мы вызываем функцию, которая считает частоты появления конкретных значений в кластере, но пропускает уникальные для всей таблицы значения. Но поскольку тут нам эта информация нужна, мы потом ее восстанавливаем в CalculateNumDistinctRgsValues.. Лучше добавить в CreateFrequencies третий параметр, который будет указывать, нужно игнорировать одиночные значения или нет и вызывать тут функцию, которая не игнорирует, станет лучше.
Можешь это поправить, если хочешь, лучше другим пулреквестом

Copy link
Collaborator

Choose a reason for hiding this comment

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

И ClusterIndex тут неправильно используется. Значение у frequencies это именно значения в таблице (их хеш если быть точным), а ClusterIndex обозначает индекс в таблице.

stats_calculator_->CalculateStatistics(lhs_pli.get(), rhs_pli.get());
}

std::shared_ptr<util::PLI const> FDVerifier::CalculatePLI(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: имхо название чуть слишком общее, как будто CalculatePLIForColumns или вроде того будет лучше

Copy link
Collaborator

@Aviu00 Aviu00 left a comment

Choose a reason for hiding this comment

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

lgtm

@polyntsov polyntsov merged commit eb1b836 into Desbordante:main Aug 8, 2023
4 checks passed
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.

3 participants