Hw 18#5
Conversation
nvaulin
left a comment
nvaulin
left a comment
There was a problem hiding this comment.
Привет!
Хорошая работа:)
По случайному лесу по логике все круто, но есть несколько косяков. Например, нету распараллеливания в предсказаниях, и в примерах всё на 1 процессе.
Также в Showcases не вижу других примеров кроме RF (-2 за каждый пропущенный из 3х).
В конце концов довольно большие проблемы с качеством кода (-6). Много проблем с неймингами, с пустыми строками, с конструкциями условий. Тут в том числе в оценку входит не просто копирование из раных домашек в 1, а какое то объединение (например у тебя импорты в центре скриптов) и исправление по тем ревью которые давали в течение года.
Тесты сами хорошие. Есть проблемы опять же по офомлению, но сами идеи классные.
В общем база годная, но если ты будешь хотеть использовать этот репозиторий при каких нибудь подачах на позиции - советую потратить вечерок на его рефакторинг:)
Баллы: 15/25 (RF) + 10/10 (Тесты) + 3/15 (Репозиторий, примеры и оформление кода) = 28
| @@ -1,143 +1,31 @@ | |||
| # my_first_tool | |||
| **This program consists of 3 tools:** | |||
| **This program consists several of my homeworks:** | |||
There was a problem hiding this comment.
В целом для внешних людей чтобы лучше понимали что происходит можно добавить что это годовая программа ИБ и курс по питону
| # my_first_tool | ||
| **This program consists of 3 tools:** | ||
| **This program consists several of my homeworks:** | ||
| ## Main script |
There was a problem hiding this comment.
Тут то что main в целом ок, хотя можно было бы дать название главному скрипту такое же как название репозитория
| from os import makedirs | ||
| import os.path | ||
| import time | ||
| import os | ||
| import shutil | ||
| import tempfile |
There was a problem hiding this comment.
Порядок импортов не очень верный:)
Ну и не надо импортировать os.path, обычно импортируют os и в нем обращаются к path
|
|
||
|
|
||
|
|
|
|
||
|
|
||
|
|
||
| class FastaRecord: |
| load_dotenv("bot.env") | ||
|
|
||
| TG_API_TOKEN = os.getenv("TG_API_TOKEN") | ||
|
|
||
| TELEGRAM_API_URL = f"https://api.telegram.org/bot{TG_API_TOKEN}/sendMessage" |
There was a problem hiding this comment.
константы тоже объявляют в начале модуля
| lenght_chech, | ||
| quality_chech, | ||
| dna_rna_tools) | ||
| #проверка чтения fastq |
There was a problem hiding this comment.
Тут эти комментарии не нужны, все же и так понятно)
| self.min_gc = 40 | ||
| self.filter_obj = FastQFilter(self.input_file, self.output_file, self.min_length, self.min_quality, self.min_gc) | ||
| def tearDown(self): | ||
| import os |
There was a problem hiding this comment.
Ну os то стоит импортировать в начале скрипта:)
| def setUp(self): | ||
| self.input_file = "test_input.fastq" | ||
| self.output_file = "test_output.fastq" | ||
| self.min_length = 50 | ||
| self.min_quality = 20 | ||
| self.min_gc = 40 | ||
| self.filter_obj = FastQFilter(self.input_file, self.output_file, self.min_length, self.min_quality, self.min_gc) | ||
| def tearDown(self): |
There was a problem hiding this comment.
Ооочень не хватает пустых строк между методами:)
| adal==1.2.2 | ||
| aiohttp==3.8.1 | ||
| aiosignal==1.2.0 | ||
| appdirs==1.4.4 | ||
| arcp==0.2.1 | ||
| argcomplete==1.8.1 | ||
| async-timeout==4.0.1 | ||
| attrs==21.2.0 | ||
| avro==1.11.0 |
There was a problem hiding this comment.
Здорово что не забыл про requirements. Но тут получается очень много всего. Можно оставлять
на основе только используемых импортов, например с помощью pipreqs.
2 participants
No description provided.