Skip to content

Hw 18#4

Open
Alisa411 wants to merge 98 commits into
mainfrom
HW_18
Open

Hw 18#4
Alisa411 wants to merge 98 commits into
mainfrom
HW_18

Conversation

@Alisa411
Copy link
Copy Markdown
Owner

@Alisa411 Alisa411 commented May 1, 2024

No description provided.

Alisa411 and others added 30 commits October 8, 2023 15:26
Copy link
Copy Markdown

@iam28th iam28th left a comment

Choose a reason for hiding this comment

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

Алиса, добрый день!

1. Рефакторинг

Readme и в целом репозиторий весьма опрятные.
Разве что примеры в ноутбуке было бы здорово снадбить пояснениями.

К коду есть ряд претензий: не везде проаннтированы типы, кое-где форматирование поехало, импорты не в правильном порядке; для последнего советую настроить и использовать isort или ruff. Тут -3б

Ещё -1б из-за неточного содержания requirements.txt

11/15

2. Лес

-1б за усложнение/дублирование кода: лучше запускать pool с одним потоком вместо отдельного if-а.
В остальном всё здорово!

24/25

3. Тесты

Тут всё хорошо, только на 2 меньше чем просили в задании, поэтому 8.4/10 баллов.


Итого у вас 43.4/50 за это дз, хорошая работа!

Успехов с защитой в ИБ и в дальнейшей карьере!

Comment thread README.md
To install fast_seqs tools you need to clone the git repository using the following command:
```bash
git clone git@github.com:Alisa411/fast_seqs.git \
cd fast_seqs
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

этого недостаточно чтоб заработало - нужно ещё зависимости через pip поставить

Comment thread HW18_main.py
Comment on lines +1 to +11
import requests
import os
import sys
import io
import datetime
from abc import ABC, abstractmethod
from dataclasses import dataclass
from random import choice
from Bio import SeqIO
from Bio.SeqUtils import gc_fraction
from bs4 import BeautifulSoup
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
import requests
import os
import sys
import io
import datetime
from abc import ABC, abstractmethod
from dataclasses import dataclass
from random import choice
from Bio import SeqIO
from Bio.SeqUtils import gc_fraction
from bs4 import BeautifulSoup
import datetime
import io
import os
import sys
from abc import ABC, abstractmethod
from dataclasses import dataclass
from random import choice
import requests
from Bio import SeqIO
from Bio.SeqUtils import gc_fraction
from bs4 import BeautifulSoup

Comment thread HW18_main.py
Methods:
__len__(): Returns the length of the biological sequence.
__getitem__(index): Gets the character at the specified index or returns a subsequence.
__str__(): Returns a string representation of the biological sequence.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

в целом эти дандеры можно наверное было особо не комментировать, они делают плюс-минус то, что от них всегда ожидается (особенно это касается __str__)

Comment thread HW18_main.py
gc_count = sum(base in 'GCgc' for base in self.sequence)
total_count = len(self.sequence)
gc_content = gc_count / total_count if total_count > 0 else 0
return gc_content * 100 if as_percentage else gc_content
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

в этих классах не хватает type hint-ов

Comment thread HW18_main.py

def transcribe(self):
transcribed_seq = ''.join(self.TRANSCRIBE_DICT.get(base, base) for base in self.sequence)
return transcribed_seq
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

дело вкуса, но я бы сразу return делал без временной переменной

к слову, прикольное использование второго аргумента в dict.get !

Comment thread custom_random_forest.py
results = [self._fit_tree(arg) for arg in args]
else:
with Pool(n_jobs) as pool:
results = pool.map(self._fit_tree, args)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

можно лишний раз не if-ать, Pool при n_jobs == 1 должен корректно отрабатывать

Comment thread custom_random_forest.py
args = [(tree, feat_ids, X) for tree, feat_ids in zip(self.trees, self.feat_ids_by_tree)]
with Pool(n_jobs) as pool:
results = pool.map(self._predict_proba_tree, args)
probas = np.sum(results, axis=0)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

здесь тоже первая if-а визуально прям сильно усложняет код

Comment thread custom_random_forest.py
n_jobs = self.n_jobs if n_jobs is None else n_jobs
probas = self.predict_proba(X, n_jobs)
predictions = np.argmax(probas, axis=1)
return predictions
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

в этом файле не достаёт аннтотаций типов

Comment thread requirements.txt
urllib3==2.2.1



Copy link
Copy Markdown

Choose a reason for hiding this comment

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

пустые строки ни к чему
и вроде бы здесь много лишнего (кажется, вы не используете pandas или scipy)

Comment thread test_module.py
url = "http://argonaute.mit.edu/cgi-bin/genscanw_py.cgi"
response = requests.post(url)

assert response.status_code == 200 No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

здесь либо переименовать тест (мол что он что именно доступность сервера проверяет), либо использовать ту же самую (глобальную) переменную , которая используется в run_genscan

потому из того, что в тесте вы захордкодили правильную ссылку, не обязательно следует, что в другом месте вы захардкодили тоже правильную ссылку)

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.

2 participants