opencobra / cobratoolbox

The COnstraint-Based Reconstruction and Analysis Toolbox. Documentation:
https://opencobra.github.io/cobratoolbox
Other
252 stars 317 forks source link

RemoveRxns does not always update model fields correctly #844

Closed stefaniamagg closed 7 years ago

stefaniamagg commented 7 years ago

I am working on a test for "removeDeadEnds", and found that the function "removeRxn" does not update the S-matrix correctly in the sample model ("DeadEndTestModel.mat"). I think this has to do with the function "removeFieldEntriesForType". @tpfau , you might know if this can be the case?

My guess is that this also has to do with the sizes of the original and reduced models. The original model has 12 metabolites and 17 reactions, then in the test 3 metabolites and 5 reactions are removed, leaving 9 metabolites and 12 reactions. The function works fine when performed on a model with different dimensions, e.g., the E. coli core model.

>> load('DeadEndTestModel.mat')
>> rxnsRem = findRxnsFromMets(model, model.mets([5; 9; 12])); %rxns that include the dead-end metabolites 5, 9, 12
>> modelRem = removeRxns(model, rxnsRem);
>> length(modelRem.mets)
ans =

     9

>> length(modelRem.rxns)
ans =

    12

>> size(modelRem.S)
ans =

    12    12

I hereby confirm that I have:

(Note: You may replace [] with [X] to check the box)

tpfau commented 7 years ago

Thanks for pointing this out. This is an stemming from both removeFieldEntriesForType and getModelFieldsForType. The latter doesn't properly check which dimension to remove, the former doesn't return S as both dimensions were matching, as I originally assumed, that this "should not happen" (well, I shouldn't assume stuff...). I'm currently testing a fix for it. and will create a PR as soon as I get it working.

laurentheirendt commented 7 years ago

Thanks @stefaniamagg and @tpfau for fixing this! :+1: