This code generates ambiguous results (due to addGeneData)

[franskloet]

consider the following example
A AND B AND C OR D AND E
With a precedence of AND over OR this should be evaluated as

((A AND B) AND C) OR (D AND E)

It is now evaluated as

((A AND B) AND (C OR D)) AND E

Which clearly gives a different result.

[cdanielmachado]

The addGeneData function comes from the method of Lee et al, (2012), and we decided to stick to the original implementation as closely as possible. Our goal was to reproduce their results.

The method doesn't account for operator precedence, so you're right, in case of ambiguity it could return an incorrect result.

However, the models we used all have explicit parenthesis around the operators, so that was not a problem for us.

I would not recommend applying this code to a model that does not have explicit operator precedence encoded with parenthesis.

Anyway, the best approach would be to deprecate the string-parsing approach and migrate to the latest sbml-fbc specification, which uses an xml-tree based representation for GPR associations.

[franskloet]

Hi Daniel,

Thanks for your answer. I guess you’re right and you should not change the sequence if so many people have used it this way already. I just stumbled upon it because I wanted to speed up the process with my own code. I’m also in contact with Brandon Barker, the one responsible at opencobra and sent him my updated code which runs faster but indeed with operate precedence. Let’s see what happens.. Anyway thanks.

Kind regards,

Frans

From: Daniel Machado [mailto:notifications@github.com]
Sent: Monday, February 8, 2016 7:04 PM
To: cdanielmachado/transcript2flux transcript2flux@noreply.github.com
Cc: Kloet, Frans van der F.M.vanderKloet@uva.nl
Subject: Re: [transcript2flux] This code generates ambiguous results (due to addGeneData) (#4)

The addGeneData function comes from the method of Lee et al, (2012), and we decided to stick to the original implementation as closely as possible. Our goal was to reproduce their results.

The method doesn't account for operator precedence, so you're right, in case of ambiguity it could return an incorrect result.

However, the models we used all have explicit parenthesis around the operators, so that was not a problem for us.

I would not recommend applying this code to a model that does not have explicit operator precedence encoded with parenthesis.

Anyway, the best approach would be to deprecate the string-parsing approach and migrate to the latest sbml-fbc specification, which uses an xml-tree based representation for GPR associations.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4#issuecomment-181440670.

[cdanielmachado]

No problem. You're welcome to contribute with a patch if you want to. I prefer not to change it myself because (1) I want to reproduce the original publication results and (2) I think it's better to encourage people to migrate to the new sbml-fbc standard to represent GPR associations.

Best regards,
Daniel