Create amino acid sequences analysis toolbox#16
Conversation
Update mrna, find seq, find res funcs
…te_isoelectric_point functions
Add two functions and update README.md
albidgy
left a comment
albidgy
left a comment
There was a problem hiding this comment.
Комментарии к работе:
- Комментарии к коммитам в большинстве своем не удались. Например, Add two def - не информативный комментарий. Лучше писать, что была добавлена такая и такая функции.
- README достаточно подробный, но по примерам непонятно, что делает программа. Здорово, что сделали блок "Common problems".
- В README нигде не сказано, как должна подаваться трехбуквенная аминокислотная последовательность. Я разобралась только читая уже сам код.
- Мне понравилось, как вы сделали главную функцию.
- Молодцы, что использовали константы.
- Достаточно часто ваш код перегружен, либо присутствует повторение одного и того же кода. Обратите внимание на комментарии, они будут полезны.
- Старайтесь не использовать конкатенацию строк, лучше добавлять элементы в список.
Так как вы написали большое количество функций, я выбрала лучшие 5 (+ главная функция) для оценивания: calculate_protein_mass, calculate_isoelectric_point, get_mrna, find_res, calculate_average_hydrophobicity. Но функции я проверила все.
Баллы:
- README - 2.2 балла (-0.5 за неинформативные примеры).
- calculate_protein_mass - 1.5 балла
- calculate_isoelectric_point - 1.5 балла
- get_mrna - 1 балл (-0.5 за конкатенацию строк)
- find_res - 1.3 балла (-0.2 за нейминг функции)
- calculate_average_hydrophobicity - 1.4 (-0.1 за отсутствие округления)
- общие штрафы: нейминг переменных -0.5 балла, большое количество повторяющегося кода -1 балл, комментарии к коммитам -0.5 балла = -2 балла.
Итого: 6.9 баллов
| else: | ||
| registr.append('Lower') | ||
| for el in seq_new: | ||
| if (len(el) != res_length): |
There was a problem hiding this comment.
| if (len(el) != res_length): | |
| if len(el) != res_length: |
| for el in seq_new: | ||
| if (len(el) != res_length): | ||
| raise TypeError('Wrong sequence format') | ||
| elif (res_length == 1): |
There was a problem hiding this comment.
| elif (res_length == 1): | |
| elif res_length == 1: |
| for el in seq: | ||
| if el.isupper(): | ||
| registr.append('Upper') | ||
| else: | ||
| registr.append('Lower') |
There was a problem hiding this comment.
Вот эту часть кода лучше было вынести в отдельную функцию. А так идет нагромождение в функции и ненужное повторение кода в if/else.
| registr.append('Upper') | ||
| else: | ||
| registr.append('Lower') | ||
| res_seq += seq |
There was a problem hiding this comment.
Конкатенация строк - это долгий и затраты процесс. Лучше добавлять элементы в список
| if query == 'three': | ||
| trans_res_seq = str() | ||
| for i in range(len(res_seq)): | ||
| if i != len(res_seq) - 1: | ||
| for three, one in RESIDUES_NAMES.items(): | ||
| if one == (res_seq[i].upper()): | ||
| trans_res_seq += three + ' ' | ||
| break | ||
| else: | ||
| for three, one in RESIDUES_NAMES.items(): | ||
| if one == res_seq[i].upper(): | ||
| trans_res_seq += three | ||
| break | ||
| res_with_reg = str() | ||
| temp_trans = [trans_res_seq[i:i + 4] for i in range(0, len(trans_res_seq), 4)] | ||
| for res, reg in zip(temp_trans, registr): | ||
| if (reg == 'Upper'): | ||
| res_with_reg += res.upper() | ||
| if (reg == 'Lower'): | ||
| res_with_reg += res.lower() | ||
| result += res_with_reg |
There was a problem hiding this comment.
Вот эту часть кода крайне сложно понять. Тут у меня есть несколько замечаний:
- if i != len(res_seq) - 1. Я понимаю, почему вы сделали иттерацию для последнего элемента отдельно, НО этого бы делать не нужно было, если бы вы записывали ответ в список, а потом просто бы сделали ' '.join(trans_res_seq).
- for three, one in RESIDUES_NAMES.items(). Не самая удачна яидея иттерироваться по словарю. Словари и хороши тем, что поиск элемента проходит за O(1), а вы делаете поиск O(n). Лучше завести второй словарь, где ключами будет однобуквенная запись аминокислотных остатков, а значения - трехбуквенными. Это ускорит ваш код, сократит его и сделает более читаемым. P.S. если вы еще не сталкивались с О, то не переживайте, вам расскажут это на дискретной математике.
- if (reg == 'Upper'). Скобки не нужны.
- for i in range(len(res_seq)). Лучше не i, а idx.
- temp_trans = [trans_res_seq[i:i + 4] for i in range(0, len(trans_res_seq), 4)]. И вот эта строка странная, потому что вы делаете дополнительный перевод строки в список. С учетом того, что до этого вы делали конкатенацию строк выше.
| elif ind == len(seq): | ||
| sum_pka += RESIDUES_CHARACTERISTICS[res][1][0] | ||
| pka_amount += 1 | ||
| pi = sum_pka / pka_amount |
| for aa in b_turn_set: | ||
| count += seq.upper().count(aa) | ||
| b_turn_exp = str(count / protein_length * 100) | ||
| res_for_seq += ['b-turn amino acids in protein' + ' is ' + b_turn_exp + '%'] | ||
| count = 0 | ||
| for aa in b_sheet_set: | ||
| count += seq.upper().count(aa) | ||
| b_sheet_exp = str(count / protein_length * 100) | ||
| res_for_seq += ['b-sheet amino acids in protein' + ' is ' + b_sheet_exp + '%'] | ||
| count = 0 | ||
| for aa in alpha_helix_set: | ||
| count += seq.upper().count(aa) | ||
| alpha_helix_exp = str(count / protein_length * 100) |
There was a problem hiding this comment.
3 цикла for - это очень неоптимально. Если вам дадут длинную аминокислотную последовательность, то код будет долго работать. Лучше сделать 1 for и написать проверку:
for aa in seq.upper():
if aa in b_turn_set:
counter_b_turn += 1
elif aa in b_sheet_set:
counter_b_sheet += 1
elif aa in counter_alpha_helix:
counter_alpha_helix += 1| function_names = {'change_residues_encoding': [change_residues_encoding, 2], | ||
| 'is_protein': [is_protein, 1], | ||
| 'get_seq_characteristic': [get_seq_characteristic, 1], | ||
| 'find_res': [find_res, 2], | ||
| 'find_site': [find_site, 2], | ||
| 'calculate_protein_mass': [calculate_protein_mass, 1], | ||
| 'calculate_average_hydrophobicity': [calculate_average_hydrophobicity, 1], | ||
| 'get_mrna': [get_mrna, 1], | ||
| 'calculate_isoelectric_point': [calculate_isoelectric_point, 1], | ||
| 'analyze_secondary_structure': [analyze_secondary_structure, 1]} |
There was a problem hiding this comment.
Интересно сделали реализацию через словари. 👍
| seqs = [change_residues_encoding(seq) for seq in args[:-1 * (function_names[procedure][1])]] | ||
| for idx, seq in enumerate(seqs): | ||
| if not is_protein(seq): | ||
| processed_result.append(f'Sequence number {idx + 1} is not available for operations! Skip it.') |
There was a problem hiding this comment.
Лучше не записывать в ответ строку с ошибкой, а выводить print() на экран, так будет нагляднее.
| return result | ||
|
|
||
|
|
||
| def run_protein_analysis(*args: str) -> Union[List[str], str]: |
There was a problem hiding this comment.
Вы не всегда возвращаете список строк или строку. Иногда вы возвращаете float и int, или их списки.
nvaulin
left a comment
nvaulin
left a comment
There was a problem hiding this comment.
Привет всем,
Попросили чекнуть тут баллы.
1) README
README в целом выглядит хорошо, хорошая структура и отдельно здорово что есть секции про установку и проблемы. Тем не менее, есть несколько моментов. Во-первых, здорово что есть примеры, но не хватает результатов работы этих примеров. То есть в идеале я могу прогнать этот код у себя и сравнить с вашим ответом. Тут я так не могу сделать. Также в примерах вы пишете вызов внутренних функций, хотя по идее пользователь ипортирует только главную и работает через неё. Также согласен с комментарием про то, что не очень ясен формат в котором требуется ввод. Такие вещи лучше было бы вынести где-то в начало. Хотя то что хотя бы в секции трабл-шутинга есть - уже ок. Я бы тут поставил не 2/2.5, а 2.3/2.5 + 0.2 за фото. Но все упомянутые вещи обязательно надо исправить.
2) Функции
get_mrna - согласен что лучше делать список чем конкатенацию строк, но все же тут у нас нет цели прям оптимизировать процесс. Код выглядит аккуратно и работает правильно, мне кажется этого пока достаточно. Но важно чтобы вы все равно знали об этом. Во-втором семестре у нас будут домашки, где вы сталкнетесь с этим, но пока для нас это не самое главное. Так что я бы поставил 1.5/1.5
calculate_average_hydrophobicity - я бы не стал снимать 0.1 балл за отсутсвие округления. Дейсвительно, может быть эти результаты вы используете при каких-то расчетах далее, и тогда вам не нужно округлять. Округление можно делать непосресдтвенно перед печатью в print. 1.5/1.5.
**3) Нейминги **
С замечаниями по неймингам переменных и функций согласен. С find_res и переменной res, конечно, мы сейчас понимаем что вы могли иметь ввиду residue. Но все таки res это максимально распространенное сокращение для result, поэтому такие штуки к содалению могут быть confusing. Так что функция find_res звучит в голове как "найди результат"
find_residue_position гораздо информативнее, вроде что-то такое Саша и предложила
**4) Общие замечания **
Со всеми замечаниями согласен. Прокомментирую структуру коммитов. Действительно, все выглядит неплохо, но все таки есть замечения:
- Сообщения коммитов (например, Add useful functions, Add two def). Есть небольшая неконсистентность, например Add modification into README и Update README.md. Коммит Remade mrna, find seq, find site funcs тоже не очень дает представление о содержании. Вы же не с нуля переписали эти функции, а просто что-то пофиксили. Ну и сообщения типа Update хороши для README, потому что там в целом просто текст. А в коде важно содержание кода, так что коммиты типа Update function хоть и кажутся ок, но информативности в них не много
- Содержание коммитов. Например, есть коммиты где за раз добавляется много всего. Если в коммитах по типу Add is_protein function and fix case-sensitive features кажется еще ок, т.к. информативное сообщение, то тот же Add useful functions оч перегружен.
- Где то содержание не соотвествует сообщению. В целом вообще правильно когда сообщение коммита строго равно его содержанию. Иногда, ладно, не страшно если заодно с коммитом исправили опечатку в другой части кода. Но у вас, нарпимер, в create templates добавлены докстринги, а в Add docstrings какой-то код пишете. В Create project structure вы сделали шаблоны функций (что круто!) но вместе с этим написаны константые словари (что тоже круто!). И то и то в принципе ок добавить в этот коммит. Но бедный словарь RESIDUES_CHARACTERISTICS был добавлен в этот коммит прямо в процессе своего появления на этот свет:)
- А еще вы видимо когда делали pull-request, решили его назвать. Тут это как раз не очень надо, так как по названиям пулл-реквестов мы смотрим где чья работа:) Вашу мы определили хотя бы по тому, что, спасибо Полине, ник на GitHub соотвествует релаьному имени. Без этого прилось бы тратить время, искать и разбираться.
Я попытался тут накидать побольше примеров, чтобы было на что обратить внимание. И обосновать почему я согласен с оценкой за структуру коммитов. Тем не менее, у вас тут не всё плохо. Небольшие недочеты, как раз на половинку балла.
Это к сожалению важно, потому что Git система не из простых, и если к ней не привыкнуть, то легко работа с ней превращается в лишнюю трату времени без пользы. Поэтому пока что польза от нее есть в виде баллов. Зато в реальной работе вы уже не будете думать над сообщениями, не будете забывать коммитить как только дописали одну функцию и т.п. Когда это все дойдет до автоматизма - вы будете сильно круче любых ваших конкурентов в биоинформатике и в целом в IT, кто не запаривался так сильно над тем чтобы освоить Git. Поэтому здесь оценка это мотивация не остановиться на "я умею git commit и помню правила написания комментариев", а двигаться дальше чтобы Git был для вас также елементарен и само-собой-разумеещь как чистить зубы по утрам. Чтобы это было для вас как искусство, и вы не только знали его, но и чувствовали.
Соре за такой опус.
В общем, скорректированный балл: 7.8
Если вы еще с чем-то не согласны, можете прям тут в ветке отвечать, всё можно обсуждать, главное чтобы все всё поняли и было понятно куда двигаться вперёд.
5 participants
No description provided.