Skip to content
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

Request from Workaholics team #24

Open
wants to merge 94 commits into
base: main
Choose a base branch
from
Open

Conversation

ivandkoz
Copy link

@ivandkoz ivandkoz commented Oct 1, 2023

The Workaholics presents new protein analysis tool.
The Workaholics team members:
Ivan Kozin
Dasha Sokolova
Yulia Volkova

ivandkoz and others added 30 commits September 25, 2023 13:53
Added new empty file protein_analysis_tool.py
Debug amino_names_transform. Debug protein_analysis input
Redisign functions molecular_weight and one_letter_to_three. Dictionaries removed from functions
Add return to get_amino_acid_sum and correct output format
Delete functions beautiful_print and reverse. Redesign codon_optimization output
Add support lenght function
Delete print from get_amino_acid_sum function
Rename 'lenght' to 'length'
Add to list of procedures get_amino_acid_sum and codon_optimization
Add contact information
yvolko and others added 25 commits October 1, 2023 09:54
Make strings in docstring shorter
Add info about brutto count to general information
Delete unnecessary returns from name_transform function
Newlines debug
Add docstring to some functions
Change name replacer_Mouse to replacer_mouse
Renamed format to letter_format
Update "how to use"
Add examples of use
Add more erros
Renamed bool functions
Debug erros output
Changed functions names in contribution part
Redesign error message in codon_optimization
Change error message of codon optimisation
Change Note to Warning
Add new chapter
Add new errors message description
Correct general info
last update
Latest update
Copy link

@albidgy albidgy left a comment

Choose a reason for hiding this comment

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

Хорошая работа!
Плюсы:

  • README написан подробно. Есть только 1 замечание: нигде не указано, как подавать аминокислотную последовательность в 3-буквенном формате.
  • Код в целом написан аккуратно, видно, что вы поработали над стилем кода.
  • Хороший нейминг.
  • Здорово, что все доступные функции записали в словарь, смотрится аккуратно.
  • Замечаний по комментариям к коммитам у меня нет, понятно, что было добавлено в каждом коммите. Я только могу посоветовать писать их в одном стиле, когда работаете в команде.

Замечания:

  • Не хватает единообразия в названии переменных. Где-то переменная с аминокислотами называется protein_sequences, где-то seqs. Лучше выбрать одно название и использовать его во всех функциях.
  • Старайтесь избегать написания одних и тех же блоков кода.

Баллы:

  • README - 2.5 балла (+0.2 за фото, -0.2 за отсутствие описания, как подавать аминокислоты в трехбуквенном формате).
  • Функции - 1.5 * 5 = 7.5 баллов
  • штрафы: функция name_transform не доделана -0.7, повторение блоков кода -1, константы не с заглавной буквы -0.1, функция is_amino_acid возвращает не True/False -0.2 = -2 балла

Итого: 9 баллов.

- List of of floats corresponding to the molecular weight in kDa
"""
molecular_weights = []
for seq in amino_acid_seqs:
Copy link

Choose a reason for hiding this comment

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

Цикл for лучше написать в главной функции. Сейчас приходится в каждой функции его писать, это ненужный повтор кода.

Comment on lines 280 to 301
amino_acid_count = {
"A": 0,
"C": 0,
"D": 0,
"E": 0,
"F": 0,
"G": 0,
"H": 0,
"I": 0,
"K": 0,
"L": 0,
"M": 0,
"N": 0,
"P": 0,
"Q": 0,
"R": 0,
"S": 0,
"T": 0,
"V": 0,
"W": 0,
"Y": 0,
}
Copy link

Choose a reason for hiding this comment

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

Лучше было погуглить и создать словарь с 0 на лету. Пример:

Suggested change
amino_acid_count = {
"A": 0,
"C": 0,
"D": 0,
"E": 0,
"F": 0,
"G": 0,
"H": 0,
"I": 0,
"K": 0,
"L": 0,
"M": 0,
"N": 0,
"P": 0,
"Q": 0,
"R": 0,
"S": 0,
"T": 0,
"V": 0,
"W": 0,
"Y": 0,
}
amino_acid_count = dict([(key, 0) for key in amino_short_names_dic.keys()])

Comment on lines 320 to 327
if cell_type == "Esherichia coli" or cell_type == "E.coli":
codon_optimization_ecoli = []
replacer_ecoli = ecoli_triplets.get
for amino_acid in range(len(protein_sequences)):
codon_optimization_ecoli += [
"".join([replacer_ecoli(n, n) for n in protein_sequences[amino_acid]])
]
return codon_optimization_ecoli
Copy link

Choose a reason for hiding this comment

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

К этой функции есть ряд замечаний:

  • Допустимые клеточные линии лучше было записать в отдельную переменную, потому что если видов 1000, а пользователь вводит не представленный вид, то сперва код пройдет 1000 if/else.
  • replacer_ecoli = ecoli_triplets.get. Не нужный этап. Вы можете напрямую обратиться к требуемому словарю.
  • for amino_acid in range(len(protein_sequences)). Код внутри for повторяется трижды, лучше было тогда вынести логику в for в отдельную функцию. Сейчас идет ненужное повторение кода.
  • Замечание по тому же циклу for. Непонятно, зачем нужно иттерироваться по индексу аминокислотного остатка, можно было просто иттерироваться по аминокислотам.

Return:
- list of int values corresponding to the length of sequences"""
result = [len(seq) for seq in seqs]
return result
Copy link

Choose a reason for hiding this comment

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

Работает верно, но можно было выдумать более интересную функцию.

Comment on lines +383 to +386
multiple_of_three.append(is_length_divisible_by_3(seq))
test_three_letters.append(is_amino_acid_three_letter(seq))
seq = seq.upper()
for letter in seq:
Copy link

Choose a reason for hiding this comment

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

Недоработана логика. Если letter_format = 1, а пользователь подает аминокислотную последовательность в 3х буквенной записи, то вернется True. Но потом это знание не используется в нижестоящем for и 3х буквенный аминокислотный остаток воспринимается как 3 аминокислоты. Если вы хотели переводить 3-буквенную запись в однобуквенную, то нужно было в for сделать шаг = 3. Либо же нужно было выбрасывать исключение, что пользователь подает в 3-буквенной записи последовательность, а letter_format == 1.

Copy link
Author

@ivandkoz ivandkoz Oct 7, 2023

Choose a reason for hiding this comment

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

Спасибо за замечание. Это бутылочное горлышко нашего проекта. Проблема в том, что иногда нельзя выяснить какую последовательность вводит пользователь. Например, если пользователь хочет ввести трехбуквеную последовательность LysLysHis, но при этом указывает letter_format = 1, то встает вопрос -- как понять, это ошибка в указании letter_format и ввод трехбуквенный или ввод все же однобуквенный (в данной последовательности каждая буква является элементом однобуквенного кода)?

Мы придумали решение, которое не решает однозначно проблему, но тем не менее может помочь избежать ошибки интерпретации при указании letter_format = 1. Мы проверяем каждую последовательность на кратность трем ее длины и на то, являются ли каждые "триплеты" последовательности трехбуквенным кодом аминокислот. При совпадении всех условий во всех заданных последовательностях, программа выводит сообщение с предупреждением : "Warning: all your sequences are similar to three-letter ones. Check the letter_format value"

Comment on lines 471 to 472
else:
return False
Copy link

Choose a reason for hiding this comment

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

Можно убрать else, потому что если код не зайдет в if, то вернет False. А если зайдет в if, вернется True и не будет уже исполнять следующую строку в коде.

Suggested change
else:
return False
return False

elif procedure == "codon_optimization":
return procedures.get(procedure)(amino_acid_seqs, cell_type)
else:
return procedures.get(procedure)(amino_acid_seqs)
Copy link

Choose a reason for hiding this comment

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

👍

Comment on lines +189 to +212
"""
Function protein_analysis:
- calculates predicted molecular weight of amino acid sequences in kDa (procedure name: molecular_weight)
- translate aa sequences from one-letter to three-letter code (procedure name: one_letter_to_three)
- calculates total amount of each amino acid in the sequences (procedure name: get_amino_acid_sum)
- makes DNA based codon optimization for the introduced amino acid sequences, support 3 types of cells:
Esherichia coli, Pichia pastoris, Mouse (procedure name: codon_optimization)
- calculates length of amino acid sequences (procedure name: length)
- counts the number of atoms of each type in a sequence (procedure name: brutto_count)

Arguments:
- one or multiple string of protein sequences written one letter or three letter code (not mixed)
- name of procedure as string
- cell type (required only for codon_optimization procedure)
- letter_format of code for the protein sequences as int: 1 for one letter, 3 for three letter code

Return:
- molecular_weight procedure returns list of floats
- one_letter_to_three procedure returns list of strings
- get_amino_acid_sum procedure returns list of dictionaries
- codon_optimization procedure returns list of strings
- length procedure returns list of int values
- brutto_count procedure returns list of dictionaries with counts of atoms in the sequence
"""
Copy link

Choose a reason for hiding this comment

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

Докстринг описано подробно, но носит более произвольных характер. Посмотрите в документации Python, как правильно его оформлять. Кстати, у вас есть хороший пример в функции molecular_weight

}


def protein_analysis(
Copy link

Choose a reason for hiding this comment

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

Обычно главная функция пишется ниже тех, которые из нее вызываются.

@@ -0,0 +1,491 @@
amino_short_names_dic = {
Copy link

Choose a reason for hiding this comment

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

Имена констант пишутся заглавными буквами.

stegodasha and others added 3 commits October 7, 2023 20:42
The outputs of the is_amino_acid function have been changed to True and False. Removed unimportant "elses"
Changing case in constants to uppercase
@albidgy
Copy link

albidgy commented Oct 16, 2023

Я пока пересматривала вашу работу заметила, что сильно штрафанула вас за повторение блоков кода. Хочу поднять балл до 9.

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.

4 participants