opencobra / memote

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

Put a cap on the number of minimal inconsistent stoichiometries #701

Closed carrascomj closed 4 years ago

carrascomj commented 4 years ago

Checklist

Is your feature related to a problem? Please describe it.

Related to #698 #697 #680.

The MILP problems performed by find_inconsistent_min_stoichiometry are producing some issues regarding timeouts and worker errors.

Describe the solution you would like.

I would suggest for putting a cap on the number of unconserved metabolites that memotes processes (this loop) to compute the minimal inconsistent stoichiometries and report it accordingly. This fix would make it easier (or possible) to use it for large networks locally or in the server.

Describe alternatives you considered

As mentioned in https://github.com/opencobra/memote/pull/688#issuecomment-652974788 It seems like using CPLEX and symengine could be recommended but an error was reported anyways.

Midnighter commented 4 years ago

If you will work on this test @carrascomj, I now think splitting it up as you had done at the very beginning is best. So I would keep one test that just checks for a consistent stoichiometry in the scored section and create two new tests in the unscored section of the report that show the inconsistent metabolites and inconsistent minimal stoichiometries.

Midnighter commented 4 years ago

697 seems to be about uploading the model and not testing it but I'm not 100% sure.

carrascomj commented 4 years ago

I agree with splitting the tests. It will also make it easier to debug if errors occurs. Oh, right, sorry, I didn't read #697 very thoroughly.

Midnighter commented 4 years ago

Definitely a very good idea to also limit the number of searches in the loop :+1:

carrascomj commented 4 years ago

After discussing it with @phantomas1234, it may be a good idea to just skip the inconsistent minimal stoichiometries test by default if it gets stuck for larger reconstructions. What do you think about that, @Midnighter?

Midnighter commented 4 years ago

Do you mean, not do the test at all or detect that it gets stuck and then skip it?

carrascomj commented 4 years ago

Not to do the test at all by default. Detecting if it gets stuck is better but I would need to first think how to to do it.

Midnighter commented 4 years ago

Would it help to introduce the solver timeout by default?

carrascomj commented 4 years ago

Oh, my fault, I thought we set a default to it but it seems like that's not the case. I was thinking that it was not having symengine installed what was giving problems. Maybe, a default solver timeout will be the solution. I will run some tests with the a problematic model on #703 (without the cap) today with the solver timeout to see if that solves it.

Midnighter commented 4 years ago

Great, thanks for looking into it. We are using a timeout on memote.io but not for the CLI.