-
Notifications
You must be signed in to change notification settings - Fork 72
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
Conversation
#include "algorithms/Fd_mine.h" | ||
#include "algorithms/Pyro.h" | ||
#include "algorithms/TaneX.h" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Наверное сюда нужно добавить algorithms/TypoMiner.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зависит от того, с каким замыслом ты создавал этот файл.
Если это перечисление вообще всех алгоритмов для облегечения инклюдов в файлах, где нужны все алгоритмы, тогда стоит добавить.
Если это только ФЗ алгоритмы, стоит переименовать -> FDAlgorithms.h
и не добавлять TypoMiner
.
Возможно, такая группировка и упрощает чтение инклюдов, но я бы наверно вообще удалил этот хэдер -- пока он только в AlgoFactory используется, можно один раз поинклюдить все алгоритмы по отдельности.
There was a problem hiding this comment.
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
и использовать транзитивное включение.
src/algorithms/TypoMiner.h
Outdated
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нужно либо реализовать прогресс, либо передавать тут пустой вектор.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 такое умееют.
There was a problem hiding this comment.
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).
#include "algorithms/Fd_mine.h" | ||
#include "algorithms/Pyro.h" | ||
#include "algorithms/TaneX.h" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зависит от того, с каким замыслом ты создавал этот файл.
Если это перечисление вообще всех алгоритмов для облегечения инклюдов в файлах, где нужны все алгоритмы, тогда стоит добавить.
Если это только ФЗ алгоритмы, стоит переименовать -> FDAlgorithms.h
и не добавлять TypoMiner
.
Возможно, такая группировка и упрощает чтение инклюдов, но я бы наверно вообще удалил этот хэдер -- пока он только в AlgoFactory используется, можно один раз поинклюдить все алгоритмы по отдельности.
There was a problem hiding this 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.
Implements the initial workflow for typo discovery using approximate and precise fd mining algorithms. Refactors
main.cpp
and adds newAlgoFactory
module --- convenient interface for creating algorithm instances. Introduces namespacealgos
(part of #63) for entities insrc/algorithms/
(eventually need to place all code fromsrc/algorithms/
into this namespace).