opencobra / memote

memote – the genome-scale metabolic model test suite
https://memote.readthedocs.io/
Apache License 2.0
127 stars 27 forks source link

Recognizing SBO annotations using lowercase "sbo". #477

Closed jonovik closed 6 years ago

jonovik commented 6 years ago

https://github.com/opencobra/memote/blob/e3316631a1f8e9d7f0f8ca7f78eb4cc6f184b71c/memote/support/sbo.py#L45

I propose to replace "SBO" with "sbo" in the code above.

My SBO annotations are not recognized because I use lowercase "sbo" as the key, not "SBO". Lowercase sbo is the identifiers.org (Juty et al. 2012) namespace for SBO, and I like to use identifiers.org whenever possible. In particular I like the immediate reference to the ontology used; for example, http://identifiers.org/sbo explains what ontology http://identifiers.org/sbo/SBO:0000249 is from.

I think identifiers.org namespaces are a better alternative than hardcoding "SBO" into sbo.py, and the metabolite and reaction annotation checks all use identifiers.org namespace-identifiers: pubchem.compound kegg.compound seed.compound inchikey chebi hmdb reactome metanetx.chemical bigg.metabolite biocyc rhea kegg.reaction metanetx.reaction bigg.reaction ec-code brenda

@ChristianLieven

jonovik commented 6 years ago

I also suggest similar changes in other files, replacing [\"\']SBO[\"\'] with 'sbo'

The change would affect the following files:

$ ag [\"\']SBO[\"\'] memote

memote/tests/test_for_support/test_for_helpers.py 224: rxn.annotation["SBO"] = "SBO:0000655"

memote/memote/support/helpers.py 165: 'SBO' in rxn.annotation and 166: rxn.annotation['SBO'] in TRANSPORT_RXN_SBO_TERMS]) 306: 'SBO' in rxn.annotation and 307: rxn.annotation['SBO'] == 'SBO:0000629'])

memote/memote/support/sbo.py 45: elem.annotation is None or 'SBO' not in elem.annotation] 69: 'SBO' not in elem.annotation or 70: elem.annotation['SBO'] not in term]

memote/tests/test_for_support/test_for_helpers.py 390: rxn1.annotation = {'SBO': 'SBO:0000629'}

memote/tests/test_for_support/test_for_sbo.py 48: met.annotation = {'SBO': 'SBO:1', 'bigg.metabolite': 'dad_2'}

ChristianLieven commented 6 years ago

Cheers @jonovik that was an oversight on our part. We want to facilitate MIRIAM-compliant behavior wherever possible.

Will be fixed in the next release!

ChristianLieven commented 6 years ago

I should also point out that the current sbo.py will likely be reworked in the future: https://github.com/opencobra/memote/issues/469

ChristianLieven commented 6 years ago

Fixed in #486. Please reopen this issue if there are any remaining problems.