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 algorithm for typo discovery #89

Merged
merged 21 commits into from
Mar 8, 2022

Conversation

polyntsov
Copy link
Collaborator

Implements the initial workflow for typo discovery using approximate and precise fd mining algorithms. Refactors main.cpp and adds new AlgoFactory module --- convenient interface for creating algorithm instances. Introduces namespace algos (part of #63) for entities in src/algorithms/ (eventually need to place all code from src/algorithms/ into this namespace).

@polyntsov polyntsov added the feature Provides new functionality label Feb 20, 2022
#include "algorithms/Fd_mine.h"
#include "algorithms/Pyro.h"
#include "algorithms/TaneX.h"

Copy link
Collaborator Author

@polyntsov polyntsov Feb 21, 2022

Choose a reason for hiding this comment

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

Наверное сюда нужно добавить algorithms/TypoMiner.h

Copy link
Collaborator

Choose a reason for hiding this comment

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

Зависит от того, с каким замыслом ты создавал этот файл.

Если это перечисление вообще всех алгоритмов для облегечения инклюдов в файлах, где нужны все алгоритмы, тогда стоит добавить.

Если это только ФЗ алгоритмы, стоит переименовать -> FDAlgorithms.h и не добавлять TypoMiner.

Возможно, такая группировка и упрощает чтение инклюдов, но я бы наверно вообще удалил этот хэдер -- пока он только в AlgoFactory используется, можно один раз поинклюдить все алгоритмы по отдельности.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Изначально создавал как хедер со всеми алгоритмами.
Кажется, что такой хедер может понадобиться в любом месте, представляющем в каком-то виде клиент для desbordante lib. На бэке веб приложения, в main консольного desborbante, в тестах desbordante. Но при этом все такие клиенты по идее должны (но в данный момент это не так) использовать AlgoFactory. Можно включить все нужные хедеры алгоритмов в AlgoFactory и использовать транзитивное включение.

template <typename PreciseAlgo, typename ApproxAlgo>
TypoMiner<PreciseAlgo, ApproxAlgo>::TypoMiner(Config const& config)
: Primitive(config.data, config.separator, config.has_header,
{"Precise fd algorithm execution", "Approximate fd algoritm execution",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Нужно либо реализовать прогресс, либо передавать тут пустой вектор.

Copy link
Collaborator

@Mstrutov Mstrutov left a comment

Choose a reason for hiding this comment

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

Выглядит очень хорошо. Я бы ещё хотел получше изучить TypoMiner и Configuration, но, если функционал уже нужен, можно мёржить.

}
}

/* Really cumbersome, also copying parameter names and types throughout the project
Copy link
Collaborator

Choose a reason for hiding this comment

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

Можно завести struct со static полями типа
public static kIsNullEqualNull = "is_null_equal_null";
или аналогичный enum, если BetterEnums такое умееют.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Была такая идея, но не понятно, нужно по struct/enum для каждого типа примитива со своими опциями или один большой struct для вообще всех возможных опций. Во втором случае наверное стоит делать что-то большее, чем просто enum, где будут еще описания опций и типы в каком-то виде (то, что сейчас захардкожено в main.cpp).

src/algorithms/FDAlgorithm.h Show resolved Hide resolved
#include "algorithms/Fd_mine.h"
#include "algorithms/Pyro.h"
#include "algorithms/TaneX.h"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Зависит от того, с каким замыслом ты создавал этот файл.

Если это перечисление вообще всех алгоритмов для облегечения инклюдов в файлах, где нужны все алгоритмы, тогда стоит добавить.

Если это только ФЗ алгоритмы, стоит переименовать -> FDAlgorithms.h и не добавлять TypoMiner.

Возможно, такая группировка и упрощает чтение инклюдов, но я бы наверно вообще удалил этот хэдер -- пока он только в AlgoFactory используется, можно один раз поинклюдить все алгоритмы по отдельности.

Copy link
Collaborator

@Mstrutov Mstrutov left a comment

Choose a reason for hiding this comment

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

OK

Separate module for algorithm instance creation helps to avoid code
duplication between places where you need to create algorithm objects,
e.g. between main() of cli desbordante app and backend of desbordante
web-app. Also this approach is a lot more scalable in terms of adding
new algorithms.
Introduce base class for FDlgorithm and move there input_generator_
field and methods and fiels to work with progress bar. These are common
things to all primitives to be implemented, such as conditional
functional dependencies, association rules and more.
Add new constructor from ColumnRelationLayoutData to algorithms
inherited from PliBasedFDAlgorithm. And make algorithms unique ownership
of relation shared. It helps to implement primitives which use
algorithms as building blocks. Such primitives need to execute multiple
algorithms over the same relation and at the same time own that
relation.
Implement methods in CSVParser to get line by its number from file. It
can be used to print result of typos miner primitive. Use boost
algorithms to parse csv string instead of manual algorithm which is
less reliable.
Improves readability
Refactor main.cpp, algo_factory.h: impelement method that creates
algorithm instance from map of parameters, it should be easier to
maintain.
Reduces code duplication and simplifies algo_factory interface.
Implement static FDAlgorithm method to convert container of fds to json.
Generally usefull functionality, e.g. for testing.
In typo mining workflow several algorithms work on the same table and
can have different RelationalSchema objects so we cannot just compare
pointers to RelationalSchema. Implement operator== for RelationalSchema
objects to address this issue.
Useful for testing when you don't have schema and therefore can't
create your own Verticals to compare with algorithm result Verticals.
It is better than handcoded version of the same functionality.
Because the progress bar is not implemented yet.
Implement script that pulls datasets using git lfs or, if git lfs
fails due to excess of free GitHub data quota, retrieves datasets from
git history. Temporary script to address small GitHub git lfs data
quota.
@Mstrutov Mstrutov merged commit 30699cc into Desbordante:main Mar 8, 2022
@polyntsov polyntsov deleted the workflow branch October 28, 2022 12:52
@Mstrutov Mstrutov mentioned this pull request Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Provides new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants