Skip to content

HW4 Zolotikov #10

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

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

Conversation

glitchheadgit
Copy link

Team:

  • Dorzhi Badmadashiev: to_rna, define_charge functions
  • Ustin Zolotikov: to_dna, define_polarity functions
  • Margarita Beskrovnaia: main, is_correct_seq, change_abbreviation functions

from typing import Dict, List, Union

# Dorzhi
def to_rna(seq: str, rna_dict: Dict[str, str] = {'F': 'UUY', 'L': 'YUN', 'I': 'AUH', 'M': 'AUG',

Choose a reason for hiding this comment

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

Словарь стоило бы вынести как константу

Comment on lines +5 to +26
'V': 'GUN', 'S': 'WSN', 'P': 'CCN', 'T': 'ACN',
'A': 'GCN', 'Y': 'UAY', 'H': 'CAY', 'Q': 'CAR',
'N': 'AAY', 'K': 'AAR', 'D': 'GAY', 'E': 'GAR',
'C': 'UGY', 'R': 'MGN', 'G': 'GGN', 'W': 'UGG'}) -> str:
"""
Converts an amino acid sequence into an RNA sequence.

Parameters
----------
seq : str
Amino acid sequence.
rna_dict : dict
Dictionary defining the correspondence of amino acids
to RNA triplets (default, standard code).
Returns
-------
str
RNA sequence.

"""
result = ''.join(rna_dict[base] for base in seq)
return result

Choose a reason for hiding this comment

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

👍👍👍 за суть операции,
но что случилось с отступами?

return result


def define_charge(seq: str, positive_charge: List[str] = ['R', 'K', 'H'],

Choose a reason for hiding this comment

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

Тут также словарь является константой, зачем его переопределять при каждом вызове функции?

Comment on lines +40 to +42
List of amino acids with positive charge (default is ['R', 'K', 'H']).
negative_charge : list
List of amino acids with negative charge (default is ['D', 'E']).

Choose a reason for hiding this comment

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

Использование списка как типа данных для поиска менее эффективно по скорости, чем множества. К тому же непонятно, зачем делать его аргументом функции - вряд ли пользователь захочет ввести свои, другие данные

Comment on lines +52 to +69
positive_count = 0
negative_count = 0
neutral_count = 0

for aa in seq:
if aa in positive_charge:
positive_count += 1
elif aa in negative_charge:
negative_count += 1
else:
neutral_count += 1

result = {
'Positive': positive_count,
'Negative': negative_count,
'Neutral': neutral_count
}
return result

Choose a reason for hiding this comment

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

Хорошее разбиение кода пустыми строками на смысловые участки, хороший вывод
С отступами проблема

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

Choose a reason for hiding this comment

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

Здорово, что тут переменные являются константами, и названы корректно! 🔥
Для задач поиска, кстати, больше подходят именно сеты, а не списки 🔥🔥🔥

На самом деле, можно было бы просто задать множества положительно, отрицательно и нейтрально заряженных полярны аминокислот, и отдельно неполярных аминокислот как константы

Для предыдущей функции эти константы можно было бы тоже эффективно использовать, и не множить сущности в коде

Comment on lines +90 to +96
polarity_count = {'Polar': 0, 'Nonpolar': 0}
for aminoacid in seq:
if aminoacid in POLAR_AA:
polarity_count['Polar'] += 1
else:
polarity_count['Nonpolar'] += 1
return polarity_count

Choose a reason for hiding this comment

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

🔥

'GLY':'G', 'HIS':'H', 'ILE':'I', 'LYS':'K', 'LEU':'L',
'MET':'M', 'ASN':'N', 'PRO':'P', 'GLN':'Q', 'ARG':'R',
'SER':'S', 'TRE':'T', 'VAL':'V', 'TRP':'W', 'TYR':'Y'}
AMINO_ACIDS_ONE_LETTER = {'A', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'K',

Choose a reason for hiding this comment

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

А можно было бы объединить сеты :)

Suggested change
AMINO_ACIDS_ONE_LETTER = {'A', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'K',
AMINO_ACIDS_ONE_LETTER = POLAR_AA.union(NONPOLAR_AA)

AMINO_ACIDS_ONE_LETTER = {'A', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'K',
'L', 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'V',
'W', 'Y'}
AMINO_ACIDS_THREE_LETTER = {'ALA', 'CYS', 'ASP', 'GLU', 'PHE',

Choose a reason for hiding this comment

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

Ну или же для получения этого множества и множества выше, взять отдельно ключи и значенися словаря ABBREVIATION_THREE_TO_ONE

Зачем вводить все вручную, если можно накодить?)

'MET', 'ASN', 'PRO', 'GLN', 'ARG',
'SER', 'TRE', 'VAL', 'TRP', 'TYR'}

import sys

Choose a reason for hiding this comment

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

Все импорты должны быть вверху

Comment on lines +163 to +166
unique_amino_acids = set(seq)
unique_amino_acids_three = set(seq.split("-"))
check = unique_amino_acids <= AMINO_ACIDS_ONE_LETTER or unique_amino_acids_three <= AMINO_ACIDS_THREE_LETTER
return check

Choose a reason for hiding this comment

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

Проблема с отступами;
советую использовать методы множеств

Suggested change
unique_amino_acids = set(seq)
unique_amino_acids_three = set(seq.split("-"))
check = unique_amino_acids <= AMINO_ACIDS_ONE_LETTER or unique_amino_acids_three <= AMINO_ACIDS_THREE_LETTER
return check
unique_amino_acids = set(seq)
unique_amino_acids_three = set(seq.split("-"))
check = unique_amino_acids.issubset(AMINO_ACIDS_ONE_LETTER) or unique_amino_acids.issubset(unique_amino_acids_three) <= AMINO_ACIDS_THREE_LETTER
return check

If several sequences are supplied, outputs the result as a list.

"""
*seqs, operation = args

Choose a reason for hiding this comment

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

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


"""
*seqs, operation = args
operations = {'one letter':change_abbreviation, 'RNA':to_rna, 'DNA':to_dna, 'charge':define_charge, 'polarity':define_polarity}

Choose a reason for hiding this comment

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

Тоже стоит вынести как константу

(отступы грустят)

operations = {'one letter':change_abbreviation, 'RNA':to_rna, 'DNA':to_dna, 'charge':define_charge, 'polarity':define_polarity}
output = []
for seq in seqs:
answer = is_correct_seq(seq.upper())

Choose a reason for hiding this comment

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

Нейминг: скорее это не ответ, а проверка, лучше назвать переменную input_check

@pavlovanadia
Copy link

pavlovanadia commented Oct 3, 2023

Хорошая работа! Мне очень понравилась, особенно ридми. Очень советую для следующих командных заданий активнее взаимодействовать по тем кускам кода, которые могут использоваться всеми членами команды)

README:

  • Отличное README! Указаны и возможные операции, и какие типы данных принимаются, и какие возвращаются, и примеры использования.

Код:

  • Хорошая типизация и докстринг
  • По сути кода: гидрофобные аминокислоты мы не можем называть нейтральными (за это баллы не снимаются)
  • Беда с отступами, 4 пробела есть не у всех функций, это прямо 😭
  • В коде очень много повторов, в частности, константных переменных (в некоторых функциях, к тому же, они являются даже не константами, а аргументами). Возникает ощущение не очень согласованной командной работы, словно не получилось договориться о пространстве используемых переменных и вынести их в константы, чтобы код был оптимален
  • Логичнее в целом константы определить в самом начале кода, а не вводить между функциями (в будущем для структуры программы учитывайте)
  • К логике и структуре кода (вот это замечание уже серьезнее): все импорты нужно писать в самом начале кода

Итог:

  • README - 2.5 балла (🔥) + 0.2 балла за командное фото
  • to_rna, define_charge, change_abbreviation - по 1 баллу (отступы)
  • define_polarity, to_dna - по 1.5 балла
  • -0.2 балла за отступы в функции protein_tool
  • -0.1 балл за импорт в середине кода
  • -0.1 балл за константы не после импортов, а в середине кода
  • UPDATE -0.5 за то, что README не в папке, прошу прощения, просмотрела при первом ревью
    Итог: 7.8

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.

3 participants