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

fix: basic information tests from memote #137

Closed
7 tasks done
BenjaSanchez opened this issue Jul 13, 2018 · 10 comments
Closed
7 tasks done

fix: basic information tests from memote #137

BenjaSanchez opened this issue Jul 13, 2018 · 10 comments
Assignees
Labels
fixed in devel this issue is already fixed in devel and will be closed after the next release memote anything related to the memote report

Comments

@BenjaSanchez
Copy link
Contributor

BenjaSanchez commented Jul 13, 2018

Description of the issue:

From the latest memote report, the following basic information tests are giving unexpected results:

  • Only 1 unique metabolite: discussed in fix: compatibility with memote #106 (comment)
  • 684 reactions appear as duplicated (due to repeated annotation ids)
  • Almost all rxns are detected as transport
  • NGAM is not detected (the test errors)

I hereby confirm that I have:

  • Tested my code with all requirements for running the model
  • Checked that a similar issue does not exist already
  • If needed, asked first in the Gitter chat room about the issue
@BenjaSanchez BenjaSanchez added the help wanted feel free to help us solving this issue! label Jul 13, 2018
@BenjaSanchez BenjaSanchez added the memote anything related to the memote report label Jul 13, 2018
@BenjaSanchez BenjaSanchez added standby work somewhere else is needed before advancing and removed help wanted feel free to help us solving this issue! labels Nov 1, 2018
@feiranl
Copy link
Collaborator

feiranl commented Nov 28, 2018

Since we add 187 SLIMEr for lipid production which are recognized as unbalanced rxns, but as they are pseudo reactions, it is difficult to balance them, is that okay for memote to read SBO terms of those reactions and recognize that they are pseudo reactions and skip the balance test for those reactions? @ChristianLieven

@BenjaSanchez
Copy link
Contributor Author

@feiranl note that this issue is pointed out in #135. Also, @ChristianLieven is on vacation until next year, but perhaps @Midnighter can help in this

@Midnighter
Copy link

Midnighter commented Nov 28, 2018

So, we are already (as one factor of multiple heuristics) identifying different types of boundary reactions and the biomass reactions by SBO terms. So if you are annotating with SBO terms we can probably add the relevant terms to the list of reactions not to test. Using the SBO term in this way should be absolutely general, though, i.e., it should be clear that it is a term for an unbalanced pseudo reaction rather than pseudo-reactions in general.

@BenjaSanchez
Copy link
Contributor Author

@Midnighter we use SBO:0000395 (Encapsulating process) for these reactions. From the definition it sounds like reactions from that category would often be unbalanced, but I'm not sure if always. Should we create an issue in memote for further discussion?

@BenjaSanchez
Copy link
Contributor Author

actually now that I look more into it SBO:0000631 sounds much more adequate for these rxns:

A conceptual process used for modeling purposes, often created solely to complete model structure, with respect to providing inflow or outflow of matter or material. Unlike other reactions, pseudoreactions are not usually subjected to mass balance considerations.

Would it make sense for memote to skip then rxns with this SBO term?

@Midnighter
Copy link

Let's move the discussion to memote so that others may chime in. We already use the child terms of SBO:0000631.

often created solely to complete model structure

This part does not sound fitting. I think 631 more describes reactions pseudoreactions that are needed purely for modeling purposes whereas lipid production describes actual biology just that it is unclear.

@BenjaSanchez
Copy link
Contributor Author

BenjaSanchez commented Nov 29, 2018

note that SLIME reactions actually do not describe lipid production, they are a formulation to represent lipid requirements, by splitting already produced lipids. So I think they could fit that description. But yes, let's continue in memote.

@feiranl
Copy link
Collaborator

feiranl commented Nov 29, 2018

Sorry for that misleading information.

@BenjaSanchez
Copy link
Contributor Author

@feiranl no worries! I have now created opencobra/memote#537 for further discussion

@BenjaSanchez BenjaSanchez self-assigned this Dec 2, 2020
@BenjaSanchez BenjaSanchez added fixed in devel this issue is already fixed in devel and will be closed after the next release and removed standby work somewhere else is needed before advancing labels Dec 2, 2020
@BenjaSanchez
Copy link
Contributor Author

Update: All these issues are solved in yeast 8.4.2, as it can be see in its memote report, with the exception of the first problem (unique metabolites), which persists due to the official model ids not being BiGG. However, it is solved if the model is transformed to be BiGG-compliant with the python wrapper:

import ComplementaryScripts.io as io
model = io.read_yeast_model(make_bigg_compliant=True)  # load with BiGG ids
io.write_yeast_model(model)  # save

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in devel this issue is already fixed in devel and will be closed after the next release memote anything related to the memote report
Projects
None yet
Development

No branches or pull requests

3 participants