Open Bastien-N opened 1 year ago
How about this?
if exist('osenseStr', 'var') % Process arguments and set up problem if isempty(osenseStr) osenseStr = 'min'; else if strcmp(model.osenseStr,'max') warning('minimizeModelFlux: model.osenseStr is max, setting to min') model.osenseStr='min'; end end else if isfield(model, 'osenseStr') if strcmp(model.osenseStr,'max') warning('minimizeModelFlux: model.osenseStr is max, setting to min') model.osenseStr='min'; end osenseStr = model.osenseStr; else osenseStr = 'min'; end end
On Thu, 6 Oct 2022 at 14:56, Bastien-N @.***> wrote:
Good afternoon,
It recently came to my attention that there is an issue with the design of minimizeModelFlux. Indeed, by default when no value is provided for its osenseStr parameter, it will be set to the value of model.osenseStr if it exists.
However, most models (at least all that I have encountered before) have model.osenseStr = "max". This causes minimizeModelFlux to, by default, maximize the flux through models. In the context of essentialRxnsTasks, this causes a very high number of reactions to be flagged as essential. This is easily fixed by changing model.osenseStr or by setting the argument in the function to "min", however due to the name of the function it feels like an confusing behavior. In addition, the function description does indicate that the default is 'max', which is actually not technically true because if the function is called as "minimizeModelFlux(model, [])", or if model.osenseStr does not exist, osenseStr is set to "min".
An important consequence is that, when using checkMetabolicTasks to find essential reactions, the results are inaccurate if the model used as input has model.osenseStr = "max", as this relies on minimizeModelFlux within essentialRxnsTasks. This is especially problematic as the inaccuracies could very easily go unnoticed.
I believe that it would be beneficial to introduce changes to make it so that minimizeModelFlux does not maximizes flux unless explicitly requested, or at least to have this behavior described in the description of checkMetabolicTasks.
Best regards, Bastien Nihant I hereby confirm that I have:
- Tried to solve the issue on my own
- Retried to run my code with the latest version of The COBRA Toolbox
- Checked that a similar issue has not already been opened
(Note: You may replace [ ] with [X] to check the box)
— Reply to this email directly, view it on GitHub https://github.com/opencobra/cobratoolbox/issues/2048, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQMEOXN4VWHJR2Y7IPCGYDWB3K7RANCNFSM6AAAAAAQ6UXFPQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>
(This message is confidential and may contain privileged information. It is intended for the named recipient only. If you receive it in error please notify me and permanently delete the original message and any copies.)
I modified it again a bit, as changing model.osenseStr is not necessary if osenseStr is provided as an argument.
if exist('osenseStr', 'var') % Process arguments and set up problem if isempty(osenseStr) osenseStr = 'min'; end else if isfield(model, 'osenseStr') if strcmp(model.osenseStr,'max') warning('minimizeModelFlux: model.osenseStr is max, setting to min') model.osenseStr='min'; end osenseStr = model.osenseStr; else osenseStr = 'min'; end end
Additionally, this modification would mean that the user receives a warning at every iteration of the checkMetabolicTask, so it may be good to also add a fix to checkMetabolicTask directly:
if isfield(model, 'osenseStr') if strcmp(model.osenseStr,'max') warning('checkMetabolicTasks: model.osenseStr is max, setting to min') model.osenseStr='min'; end end
Good afternoon,
It recently came to my attention that there is an issue with the design of minimizeModelFlux. Indeed, by default when no value is provided for its osenseStr parameter, it will be set to the value of model.osenseStr if it exists.
However, most models (at least all that I have encountered before) have model.osenseStr = "max". This causes minimizeModelFlux to, by default, maximize the flux through models. In the context of essentialRxnsTasks, this causes a very high number of reactions to be flagged as essential. This is easily fixed by changing model.osenseStr or by setting the argument in the function to "min", however due to the name of the function it feels like an confusing behavior. In addition, the function description does indicate that the default is 'max', which is actually not technically true because if the function is called as "minimizeModelFlux(model, [])", or if model.osenseStr does not exist, osenseStr is set to "min".
An important consequence is that, when using checkMetabolicTasks to find essential reactions, the results are inaccurate if the model used as input has model.osenseStr = "max", as this relies on minimizeModelFlux within essentialRxnsTasks. This is especially problematic as the inaccuracies could very easily go unnoticed.
I believe that it would be beneficial to introduce changes to make it so that minimizeModelFlux does not maximizes flux unless explicitly requested, or at least to have this behavior described in the description of checkMetabolicTasks.
Best regards, Bastien Nihant I hereby confirm that I have:
(Note: You may replace [ ] with [X] to check the box)