-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
This code generates ambiguous results (due to addGeneData) #4
Comments
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. |
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] 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. — |
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, |
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.
The text was updated successfully, but these errors were encountered: