-
Notifications
You must be signed in to change notification settings - Fork 43
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: compatibility with memote #106
Comments
Super excited to hear that and to soon see memote in action with an important model. Some comments:
Reading of SBO terms will be fixed when opencobra/cobrapy#685 gets merged. I'm trying to wrap up the open PRs on cobrapy but I can't give you an ETA. It will also require memote to know the full SB ontology which it currently doesn't. So maybe those features can land at the same time. Apart from that we still need to make the memote report a bit more resilient so that errors when they do occur do not break the formatting.
Very curious, we'll have to look into that.
Funny, probably also related to the SBML parser. |
@Midnighter solving issue #19 actually solved many of the erroring tests. Commenting on some remaining ones:
The only unique metabolite is
When looking at the 3rd line of the SBML file:
Not sure about these distinctions, is there a reason to parse |
Good one! Yes that's the issue precisely! The code in memote is: def find_unique_metabolites(model):
"""Return set of metabolite IDs without duplicates from compartments."""
# TODO: BiGG specific (met_c).
return set(met.id.split("_", 1)[0] for met in model.metabolites) We'll try to find a solution here that handles this appropriately! |
@ChristianLieven maybe comparing same |
Can you try again with memote 0.7.4, please? We just merged code that finds duplicate metabolites based on annotation. |
I'm not an expert on SBML so I may be quoting the wrong place here:
And:
As far as I understand, the |
Name is a good option! Checking the ID is unrealiable, although unfortunately the name too is optional and hence may be an issue in other models. I can't really think of a good solution right now, will have to think about it some more.
|
Without having read the full discussion:
Does memote require BiGG IDs to be used? If not, I would suggest to look for unique metabolites by comparing annotations. If two |
It should not, or at least I thought I had removed all requirements as to its usage. This one particular test has slipped past and will be fixed ASAP. Again, thanks @BenjaSanchez for spotting this! And, @tpfau indeed we'll compare the annotations now as this is already how we determine duplicate reactions. |
@tpfau at the moment @matthiaskoenig is writing a new SBML parser for cobrapy and he wants to do the same thing. Store SBML model IDs, make them Python compliant, restore the SBML IDs upon writing the model. |
@Midnighter ftr I tried it with memote 0.7.4 and the problem persists. On the other hand no duplicate metabolites are detected, so that at least works well :) @tpfau before I was referring to the model id and metaid: If one updates the COBRA field |
@BenjaSanchez: |
Having said that: |
How is this going Ben? Do you need anything specific still from our side? |
@ChristianLieven there's an open PR #132 to include SBO terms, which solves several of the problems stated here (even though it creates some new ones), could you take a look and give us feedback? |
Description of the issue:
We would like to include memote as provider of QC, as we are about to include many new genes/reactions/mets, and it would be best to see how the model changes as we include them. In order to do that we need to:
Please comment here if anyone finds more problems using memote @hongzhonglu @feiranl @edkerk @ChristianLieven @Midnighter
I hereby confirm that I have:
The text was updated successfully, but these errors were encountered: