-
Notifications
You must be signed in to change notification settings - Fork 0
Implement sorensen_dice algorithm #14
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
base: main
Are you sure you want to change the base?
Conversation
|
Сделал привязку алгоритма соренсена. Поправил алгоритм так, как сказал @LiceyMaxim |
ARQtty
left a comment
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.
Нужен рефактор алгоритмов сравнения
models/sorensen/sorensens_dice.py
Outdated
| @@ -0,0 +1,9 @@ | |||
| def comparison(first_phrase: str, second_phrase: str) -> float: | |||
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.
Раньше у нас был план сделать базовый класс алгоритма проверки. Сейчас добавился третий алгоритм сравнения, и всё начало быть несистемно и хаотично. Сделай пожалуйста базовый класс проверки в models/ и отнаследуй от него все текущие алгоритмы сравнения
- Create BaseAlgorithm - Inherit algorithms from it - Rename test files to single standart
|
ARQtty
left a comment
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.
Базовый Алгоритм должен быть только один. Нужно переместить бизнес-логику работы с файлами лабы в специальный для этого класс, а сравнение лаб как строк оставить в models/
api/utils/scoring/base_check.py
Outdated
| class BaseCheck(): | ||
|
|
||
| def __init__(self) -> None: | ||
| def __init__(self, comparison_method: BaseAlgorithm.comparison) -> None: |
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.
не стоит прокидывать конкретный метод класса. Достаточно прокинуть класс
api/utils/scoring/base_check.py
Outdated
| raise NotImplementedError | ||
| return self.comparison_method(code, lab) | ||
|
|
||
| def name(self) -> str: |
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.
тут можно сделать магию вроде return self.__class__.__name__
api/utils/scoring/base_check.py
Outdated
| from models.base_algorithm import BaseAlgorithm | ||
|
|
||
|
|
||
| class BaseCheck(): |
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.
Базовая Проверка? Что за проверка? Вне контекста непонятно. Базовый Алгоритм Сравнения был бы более трактуем
api/utils/scoring/jaccard.py
Outdated
|
|
||
| def name(self) -> str: | ||
| return self.__class__.__name__.lower() | ||
| return self.__class__.__name__.lower()[:-5] |
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.
хочется не заниматься таким переопределением. Можно делать это в базовом классе
- Move algorithm implementation to models
|
Перекинул обработку алгоритмов в models |
| self.connector = SQLiteConnector() | ||
| self.levenshtein = Levenshtein() | ||
| self.jaccard = Jaccard() | ||
| self.methods = [ |
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.
тут не хватает коммента о том, зачем сохранять именно такие туплы
|
ARQtty
left a comment
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.
гуд. Чуть уточнить типы - и можно мержить
|
Поправил типизацию |
No description provided.