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

Completed HW4 task #18

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

Conversation

estakathersun
Copy link

No description provided.

Copy link

@SidorinAnton SidorinAnton left a comment

Choose a reason for hiding this comment

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

В целом неплохо!
Самый важный момент -- вы все время возвращали строки. Скорее всего вы это делали, чтобы их распечатывать )))
Однако вот представьте, что вы пишете функцию, где вам нужно использовать посчитанные аминокислоты. И вроде у вас даже есть функция для подсчета... но она почему-то возвращает f'Amino acids freq of the sequence {seq}: {amino_acid_count}' :/

Распечатать-то всегда можно ... Можно написать даже отдельную функцию для того, чтобы печатать )))
А вообще молодцы!

Comment on lines +204 to +205
if operation == '':
raise ValueError('Operation value is not specified')

Choose a reason for hiding this comment

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

А зачем это проверять? ))
В таком случае operation нужно вызывать всегда (это теперь тип keyword_only_args)

Comment on lines +208 to +213
for seq in seqs:
is_peptide(seq)
output = ''
for seq in seqs:
output += operation_dict[operation](seq)
output += '\n\n'

Choose a reason for hiding this comment

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

  1. Почему бы is_peptide не проверять в нижнем for'е? )))
  2. Зачем output делать строкой а потом добавлять "\n\n"?
    По сути этот код нужно было написать вот так:
output = []
for seq in seqs:
    is_peptide(seq)
    output.append(operation_dict[operation](seq))

Copy link
Author

Choose a reason for hiding this comment

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

  1. Идея была в том, чтобы сначала сделать проверку последовательностей, и после нее уже вызывать функции. Типа, чтобы после обработки 188-ой последовательности из 190 не вывалилась бы ошибка и все оказалось бы впустую...
    Либо как-то обыграть эту историю так, чтобы ошибка тоже записывалась? Например, записывать результаты выполнения функции в словарь в виде { 'sequence' : result } и сюда же записать ошибку в случае ее возникновения? Просто чтобы в любом случае функция сделала какой-то return с тем, что выполнено...

Choose a reason for hiding this comment

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

Вот когда у нас будут ошибки, тогда и разберемся )))

Comment on lines +175 to +178
all_aminoacids = {
'A', 'R', 'N', 'D', 'C', 'H', 'G', 'Q', 'E', 'I',
'L', 'K', 'M', 'P', 'S', 'Y', 'T', 'W', 'F', 'V'
}

Choose a reason for hiding this comment

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

Нужно было сделать константой (ONE_LETTER_AMINOACIDS) и передвинуть вверх

Comment on lines +183 to +185
if set(seq).issubset(all_aminoacids): # if set(seq) <= all_aminoacids
return True
raise ValueError(f'Incoming sequence {seq} is not a peptide')

Choose a reason for hiding this comment

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

А зачем тут кидать ошибку, если мы можем вернуть False? )))
Ну то есть буквально is_peptide --> True / False. У нас же не какой-нибудь check_...
Ну и в таком случае можно было бы сразу сделать

return set(seq).issubset(all_aminoacids)

Comment on lines +158 to +163
motif_dict = {
'Caspase 3': [['D'], ['M'], ['Q'], ['D']],
'Caspase 6': [['V'], ['E'], ['H', 'I'], ['D']],
'Caspase 7': [['D'], ['E'], ['V'], ['D']],
'Enterokinase': [['D', 'E'], ['D', 'E'], ['D', 'E'], ['K']]
}

Choose a reason for hiding this comment

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

А вот это уже какая-то специфичная штука для функции get_cleavage_sites. Непонятно, почему она тут, а не в теле функции

Comment on lines +60 to +65
if hydrophobicity_sum > 0:
return f"Sequence {sequence}: Hydrophilic"
elif hydrophobicity_sum < 0:
return f"Sequence {sequence}: Hydrophobic"
else:
return f"Sequence {sequence}: Neutral"

Choose a reason for hiding this comment

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

Ну и вот опять же, лучше возвращать уж hydrophobicity_sum. Или значения Hydrophilic, Hydrophobic, Neutral

weight = 18.02 # for the H and OH at the termini
for amino_acid in seq:
weight += amino_acid_weights[amino_acid]
return f'Molecular weight of the sequence {seq}: {round(weight, 2)} Da'

Choose a reason for hiding this comment

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

И опять return weight

from typing import Dict


def calculate_percentage(seq: str) -> str:

Choose a reason for hiding this comment

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

неудачно название :(
буквально "посчитать процент" ... чего? )))

Comment on lines +19 to +22
amino_acid_percentages = {} # dict to store each amino acid and its %
for amino_acid, count in amino_acid_counts.items():
percentage = round(((count / total_amino_acids) * 100), 2)
amino_acid_percentages[amino_acid] = percentage

Choose a reason for hiding this comment

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

В целом-то вы могли сразу пересчитывать значения в amino_acid_counts

for amino_acid, count in amino_acid_counts.items():
percentage = round(((count / total_amino_acids) * 100), 2)
amino_acid_percentages[amino_acid] = percentage
return f'Amino acids percentage of the sequence {seq}: {amino_acid_percentages}'

Choose a reason for hiding this comment

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

Ну и еще раз -- return amino_acid_percentages

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