Development #1
Open
Development #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
albidgy
reviewed
Oct 16, 2023
albidgy
left a comment
albidgy
left a comment
There was a problem hiding this comment.
Плюсы:
- Молодец, что добавила в README примеры, что вернется если подать такую-то последовательность.
- Код, который ты пишешь, работает правильно.
- Докстринги и typing сделаны правильно и подробно. Здорово!
Замечания:
- Не забывай ставить пробелы.
- Не включила границы в fastq_analysis.
- Не нужно системные файлы добавлять на GitHub, это плохая практика. Ты добавила директорию .ipynb_checkpoints.
- Вот совсем непонятно, что было сделано за тот или иной коммит. Хочется более конкретных комментариев, что было добавлено/сделано.
- У тебя достаточно тяжело воспринимаемый код получается. Это сейчас не страшно, но над этим нужно работать. У меня есть 2 предложения:
- Когда ты написала какую-то функцию, пробуй придумать, как можно убрать повторяющиеся части кода (если они есть).
- Можно, думаю, смотреть прошлые домашние задания одногруппников. Мы там указываем на преимущества и недостатки. Анализируй код, почему он тебе нравится или не нравится.
Баллы:
- 3 фильтрации FASTQ 3/3
- Главная функция 0.9/1 (границы значений не включены)
- README 2/2
- Структура репозитория и качество кода 2/3 (-0.5 за системные файлы, -0.3 за комментарии к коммитам, -0.2 Отсутствие пробелов).
Улучшение кода ДНК/РНК и белковых тулов 0.8/1.
Итого: 8.7 баллов
| :rtype: str | ||
| :return: complement sequence | ||
| """ | ||
| complement_dict = {'A': 'T', 'C': 'G', |
There was a problem hiding this comment.
Про словарь я тебе писала, что его лучше сделать константой и вынести за пределы функции.
| complement_dict = {'A': 'T', 'C': 'G', | ||
| 'G': 'C', 'T': 'A', 'U': 'A', 'a': 't', | ||
| 'c': 'g', 'g': 'c', 't': 'a', 'u': 'a'} | ||
| complement_seq = [] |
| :return: GC-contentn percent | ||
| """ | ||
| length = len(seq) | ||
| gc_content = 0.0 |
There was a problem hiding this comment.
Можно не создавать переменную, она сама создастся на лету в строке 78
| length = len(seq) | ||
| gc_content = 0.0 | ||
| seq_up = seq.upper() | ||
| c = seq_up.count("C") |
| seq_up = seq.upper() | ||
| c = seq_up.count("C") | ||
| g = seq_up.count("G") | ||
| gc_content = round(((c+g)/length*100),2) |
There was a problem hiding this comment.
Не забывай про пробелы. + одна пара скобок тут лишняя.
Suggested change
| gc_content = round(((c+g)/length*100),2) | |
| gc_content = round((c+g) / length * 100 , 2) |
| if len(sep_seq[0]) == 1: | ||
| encode_seq.append(RESIDUES_NAMES_THREE[residue]) | ||
| for residue, reg in zip(encode_seq, save_register(seq)): | ||
| if (reg == 1): |
| encode_seq.append(RESIDUES_NAMES_THREE[residue]) | ||
| for residue, reg in zip(encode_seq, save_register(seq)): | ||
| if (reg == 1): | ||
| encode_seq_registered += residue.upper() |
| fin_seq.append(residue) | ||
| if ((i+1) % 3 == 0): | ||
| fin_seq.append(' ') | ||
| return ' '.join(fin_seq) |
There was a problem hiding this comment.
Suggested change
| return ' '.join(fin_seq) | |
| return ''.join(fin_seq) |
Лучше убрать пробел, а то запись странная получается.
Код работает правильно, но в нем много повторений. Если тебе будет интересно, как это можно сделать короче, напиши мне, я напишу альтернативную версию.
Comment on lines
+176
to
+178
| residue_count[[tl_code for tl_code in RESIDUES_NAMES if RESIDUES_NAMES[tl_code] == residue][0]] = 0 | ||
| for residue in seq: | ||
| residue_count[[tl_code for tl_code in RESIDUES_NAMES if RESIDUES_NAMES[tl_code] == residue][0]] += 1 |
There was a problem hiding this comment.
Вот эту часть можно было переписать. 2 словаря у тебя теперь есть, поэтому достаточно легко сделать это более аккуратно. Замечание не исправлено.
| site_full_position.append(f'{site_start_position[counter]}:{site_end_position[counter]}') | ||
| return f'Site entry in sequence = {site_count}. Site residues can be found at positions: {site_full_position}' | ||
| else: | ||
| return f'{site} site is not in sequence!' |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request adds a utility Bio_Seq_Analysis_Tool.py with 3 main functions, a folder with 3 modules required for Bio_Seq_Analysis_Tool and README.md file