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

Refactor TANE-based algorithms #11

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Refactor TANE-based algorithms #11

wants to merge 10 commits into from

Conversation

iliya-b
Copy link
Owner

@iliya-b iliya-b commented Mar 20, 2024

Generalize Tane and PFDTane, add additional tests.

In order to check if the refactoring caused any performance loss, following experiments were performed.
The discovery task was run as cli.py --task=afd --algo=tane --error=0.01 --table=... with new and original versions of TANE implementation. Following heavy datasets were utilized: EpicMeds.csv, adult.csv, EpicVitals.csv.

Following list demonstrates measured running time of the old and new algorithms, correspondingly (confidence intervals of 95%, with 8-11 iteration):

  1. EpicMeds old (60.35218181818182 +- 0.12249305120263862)
  2. EpicMeds new (60.148099999999985 +- 0.19678497805104167)
  3. adult old (24.567 +- 0.05169441635645702)
  4. adult new (24.69025 +- 0.02987949934351148)
  5. EpicVitals old (10.87425 +- 0.1395892093087158)
  6. EpicVitals new (10.870625 +- 0.16149766469774526)

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/core/algorithms/fd/tane/tane.h Outdated Show resolved Hide resolved
@iliya-b iliya-b force-pushed the pfdtane-generalize branch from 83dee44 to 9b3495c Compare March 22, 2024 21:42
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/core/algorithms/fd/tane/tane.h Show resolved Hide resolved
@iliya-b iliya-b force-pushed the pfdtane-generalize branch 2 times, most recently from c70c5bc to 0a0ef54 Compare March 23, 2024 13:08
@iliya-b iliya-b force-pushed the pfdtane-generalize branch from 0a0ef54 to d6b8714 Compare April 11, 2024 11:08
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/tests/test_pfdtane.cpp Outdated Show resolved Hide resolved
src/tests/test_pfdtane.cpp Outdated Show resolved Hide resolved
src/tests/test_pfdtane.cpp Outdated Show resolved Hide resolved
src/tests/test_pfdtane.cpp Outdated Show resolved Hide resolved
@iliya-b iliya-b force-pushed the pfdtane-generalize branch 4 times, most recently from 74d5672 to ca769bb Compare April 17, 2024 15:19
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/core/algorithms/fd/tane/tane_common.cpp Outdated Show resolved Hide resolved
@iliya-b iliya-b force-pushed the pfdtane-generalize branch 7 times, most recently from 98159e0 to 554d08c Compare April 18, 2024 13:59
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/core/algorithms/fd/tane/tane.h Outdated Show resolved Hide resolved
src/core/algorithms/fd/tane/tane.h Outdated Show resolved Hide resolved
@iliya-b iliya-b force-pushed the pfdtane-generalize branch 3 times, most recently from 13016d9 to 84cf071 Compare April 18, 2024 22:25
@iliya-b iliya-b force-pushed the pfdtane-generalize branch 6 times, most recently from 0b202ac to 5ea3e11 Compare September 10, 2024 13:47
`clang-format-diff` errors out in case of an error, not executing the
following commands in the script
The old version does not contain the typing header.
Note that calling algorithms is not thread-safe. However, this allows
algorithms to call Python code from multiple threads.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -26,7 +26,7 @@ INITIALIZE_EASYLOGGINGPP

namespace python_bindings {

PYBIND11_MODULE(desbordante, module) {
PYBIND11_MODULE(desbordante, module, pybind11::mod_gil_not_used()) {

Choose a reason for hiding this comment

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

warning: invalid case style for variable 'PYBIND11_MODULE' [readability-identifier-naming]

Suggested change
PYBIND11_MODULE(desbordante, module, pybind11::mod_gil_not_used()) {
pybin_d11_module(desbordante, module, pybind11::mod_gil_not_used()) {

@iliya-b iliya-b force-pushed the pfdtane-generalize branch 4 times, most recently from 646bc0f to 400f9e8 Compare September 23, 2024 10:54
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