opencobra / memote

memote ā€“ the genome-scale metabolic model test suite
https://memote.readthedocs.io/
Apache License 2.0
128 stars 28 forks source link

Generalize Metabolite IDs in test_detect_energy_generating_cycles #755

Closed hgscott closed 1 year ago

hgscott commented 1 year ago

When I use MEMOTE on my models (generated in KBase), all of the "Erroneous Energy Generating Cycles" tests are skipped. image

I determined that the problem was that my model's metabolites were not using the MetaNetX ID as the main ID, but most (>90%) of metabolites have a MetaNetID in their annotation field. Because the MetaNetID is not use the metabolite ID, the tests were all being skipped because of this if statement. https://github.com/opencobra/memote/blob/4c08f46849628507ac8c4de56b5d6ec745feed0b/src/memote/suite/tests/test_consistency.py#L156-L160

However, since the detect_energy_generating_cycles function retrieves the metabolites using the find_met_in_model function rather than just the ID, you can use the same function in the test_detect_energy_generating_cycles function to check that the metabolite is there before proceeding, such as:

main_comp = helpers.find_compartment_id_in_model(model, "c")
    try:
        helpers.find_met_in_model(model, met, main_comp)[0]
    except:
        pytest.skip(
            "This test has been skipped since metabolite {} could "
            "not be found in the model.".format(met)
        )

Doing that yielded the following results for my own model: image

I have made these changes on a branch of my own fork of the repo, and can open a pull request if this change would be helpful to others: https://github.com/hgscott/memote/tree/cycles_met_ids

Midnighter commented 1 year ago

Hi @hgscott,

Thank you for the report! Any contributions are always welcome, so please do open a PR. There is the further complication, that some of the MNX IDs may have been updated and we should search for current and deprecated IDs... But any improvement is welcome šŸ˜ƒ

hgscott commented 1 year ago

Hi, @Midnighter! I don't think I am ready to tackle the current vs. deprecated MNX ID problem, but I have opened #756 to incorporate my changes to that if statement. Let me know if there is anything else I can help with, thanks!