Skip to content

Review MRPS7#33

Open
nvaulin wants to merge 1 commit intomainfrom
MRPS7
Open

Review MRPS7#33
nvaulin wants to merge 1 commit intomainfrom
MRPS7

Conversation

@nvaulin
Copy link
Copy Markdown
Member

@nvaulin nvaulin commented Feb 26, 2024

Review MRPS7

Comment thread MRPS7.py
Comment on lines +88 to +89
return sum(amino_acid_weights.get(aa, 0) for aa in self._sequence)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Код не учитывает потерю воды при образовании аминокислотной цепи. Например,

        water_mw = 18
        for aa in list_input_seq:
            total_mw = sum(aa_weight_dict[a] for a in list_input_seq)
            mw_water_removed = (total_mw - (water_mw * (len(list_input_seq)-1)))
        return mw_water_removed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Отличное замечание. Я бы его еще оформил с подсветкой синтаксиса:
image

Comment thread MRPS7.py
Comment on lines +45 to +46
gc_count = sum(base in {'G', 'C'} for base in self._sequence)
return gc_count / len(self._sequence)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Поскольку это абстрактный класс, то методов здесь не должно быть

Comment thread MRPS7.py
raise ValueError("Invalid sequence for the given type.")
self._sequence = sequence

@abstractmethod
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Насколько я понимаю, декоратор @abstract должен быть перед def init

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Да, тут вообще все методы должны быть абстрактными

Comment thread MRPS7.py
Comment on lines +76 to +78
valid_amino_acids = {'A', 'R', 'N', 'D', 'C', 'Q', 'E', 'G', 'H', 'I',
'L', 'K', 'M', 'F', 'P', 'S', 'T', 'W', 'Y', 'V'}
return all(amino_acid in valid_amino_acids for amino_acid in sequence)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Можно было добавить ситуацию когда подаются не заглавные буквы, например c помощью set:

    unique_chars = set(seq)
    single_letter = set('GALMFWKQESPVICYHRNDTgalmfwkqespvicyhrndt')

Comment thread MRPS7.py
Comment on lines +51 to +52
def is_valid(self, sequence: str) -> bool:
return all(nucleotide in {'A', 'C', 'G', 'T'} for nucleotide in sequence)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

То же самое про заглавные и строчные буквы, можно добавить такую проверку:

        nucleotides_dna = set('ATGCatgc')
        nucleotides_rna = set('AUGCaugc')

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.

2 participants