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

Mit expression #5

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Conversation

OmidOftadeh
Copy link
Contributor

Mitochondrial expression system

@OmidOftadeh
Copy link
Contributor Author

OmidOftadeh commented Feb 13, 2020

Hi Pierre,
Indeed, I have changed 3 files to implement mitochondrial RNAP and ribosome.
Here is a description of changes in each file:

  1. genes.py: the setters for transcribed_by and translated_by attributes are changed so they can be set if either the gene is not connected to a model or it is connected to a model, but synthesis constraint is not created yet.
  2. memodel.py: two functions, add_translation_by and add_transcription_by were added to assign the corresponding RNAP and ribosome to each gene, respectively. Moreover, _populate_ribosome and _populate_rnap were modified to define a total capacity constraint per each ribosome and RNAP.
  3. dict.py: it was modified so that regardless of type of transcribe_by and translated_by (either enzyme object or string) it can be converted to JSON file.

Cheer,
Omid

EDIT (by Pierre): formatting

@psalvy psalvy added the enhancement New feature or request label Feb 13, 2020
@psalvy psalvy self-assigned this Feb 13, 2020
@psalvy psalvy self-requested a review February 13, 2020 16:30
@@ -14,6 +14,7 @@
from cobra import Gene
from Bio.Seq import Seq
from Bio.Alphabet import DNAAlphabet, RNAAlphabet, ProteinAlphabet
from etfl.optim.constraints import SynthesisConstraint
Copy link
Member

Choose a reason for hiding this comment

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

change to
from ..optim.constraints import SynthesisConstraint

else:
# We need to make the model change the corresponding cstr
mod_id = self.id + '_transcription'
Cstr = self.model.get_constraints_of_type(SynthesisConstraint)
Copy link
Member

Choose a reason for hiding this comment

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

please respect the conventions and use Uppercas names only for classes. variables are lowercase most of the time.

try:
Cstr.get_by_id(mod_id)
except KeyError:
cstr_exist = 0
Copy link
Member

Choose a reason for hiding this comment

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

Please use True and False instead of 1 and 0

Copy link
Member

Choose a reason for hiding this comment

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

Also please add comments as to why you are doing this: checking that the genes are not transcribed yet.

self._transcribed_by = value
else:
# We need to make the model change the corresponding cstr
mod_id = self.id + '_transcription'
Copy link
Member

Choose a reason for hiding this comment

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

Please replace by get_transcription_reaction_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean I should write a new function to do this?

Copy link
Member

Choose a reason for hiding this comment

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

There is already a function for this here

cstr_exist = 0
if cstr_exist:
# trancription constraint already exists
raise NotImplementedError()
Copy link
Member

Choose a reason for hiding this comment

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

Please add #TODO with instructions

@@ -86,7 +107,27 @@ def translated_by(self):
@translated_by.setter
def translated_by(self,value):
# TODO: Make this a setter that rewrites the adequate constraints
raise NotImplementedError()
if value != self._translated_by:
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as before

@@ -259,24 +259,54 @@ def add_nucleotide_sequences(self, sequences):
:param sequences:
:return:
"""

Copy link
Member

Choose a reason for hiding this comment

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

can you please remove these empty lines containing only spaces


def add_transcription_by(self, transcription_dict):

for gene_id, transcripted_by in transcription_dict.items():
Copy link
Member

Choose a reason for hiding this comment

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

Please rename transcripted_by into transcribed_by

# transcripted_by is a list of rnap(s)
try:
self.genes.get_by_id(gene_id).transcribed_by = transcripted_by
except KeyError:
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment explaining in which case this error would happen


def _make_peptide_from_gene(self, gene_id):
free_pep = Peptide(id=gene_id,
name='Peptide, {}'.format(gene_id),
gene_id=gene_id)
free_pep._model = self
self.peptides += [free_pep]

def add_peptide_sequences(self, aa_sequences):
Copy link
Member

@psalvy psalvy Feb 24, 2020

Choose a reason for hiding this comment

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

Can you remind me for which case you had to add this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a function that I have to add my peptide sequences. It's not related to mitochondrial expression. You can neglect it.

for x in self.rnap]
# only for genes trascribed by this rnap
sum_RMs = symbol_sum([x.unscaled for x in all_rnap_usage \
if x.hook.transcribed_by is None \
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment explaining why we include the None as well

- sum([x.concentration for x in self.rnap.values()])
usage /= min([x.scaling_factor for x in self.rnap.values()])
- sum([self.rnap[rnap_id].concentration])
usage /= min([self.rnap[rnap_id].scaling_factor])
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the min here, we do not have an iterable anymore

# The total RNAP capacity constraint looks like
# ΣRMi + Σ(free RNAPj) = Σ(Total RNAPj)
usage = sum_RMs \
+ sum([x.unscaled for x in free_rnap]) \
- sum([x.concentration for x in self.rnap.values()])
usage /= min([x.scaling_factor for x in self.rnap.values()])
- sum([self.rnap[rnap_id].concentration])
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the sum here, we do not have a list anymore

sum_RPs = symbol_sum([x.unscaled for x in all_ribosome_usage])
# only for genes traslated by this ribosome
sum_RPs = symbol_sum([x.unscaled for x in all_ribosome_usage \
if x.hook.translated_by is None \
Copy link
Member

Choose a reason for hiding this comment

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

same as before

if gene.translated_by is not None else None
except AttributeError:
obj['translated_by'] = [x for x in gene.translated_by] \
if gene.translated_by is not None else None
Copy link
Member

Choose a reason for hiding this comment

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

I see no method to read that back from the dictionary ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do this later ;)

etfl/io/dict.py Outdated
g._rna = Seq(gene_dict['rna'], alphabet=RNAAlphabet())
g._peptide = Seq(gene_dict['peptide'], alphabet=ProteinAlphabet())
g._copy_number = int(gene_dict['copy_number'])

Copy link
Member

Choose a reason for hiding this comment

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

empty line

Copy link
Member

@psalvy psalvy left a comment

Choose a reason for hiding this comment

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

Please go through the comments and implement the requested fixes. In particular:

  • You need to comment more your code

  • There is no method to read the transcribed_by / translated_by back from the dictionary upon serialization

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants