Closed jmrohwer closed 2 years ago
fbergmann
commented
2 years ago at first guess, i would blame the deep cloning of the original model. since we are just using the python way of doing things here. if some of the libSBML ASTNode objects would be held by the original document (rather than say an infix form), deep cloning might not be able to copy elements correctly.
jmrohwer
commented
2 years ago Then why does it work (without fail) when I execute it locally?
JR-1991
commented
2 years ago When a ModelGenerator instance is called, variables in the equation attribute are encapsulated in ' so that on addition to the EnzymeMLDocument these can be identified and checked for consistency to the document. Furthermore, the intend was to give users the opportunity to add variables using their actual names rather than plain IDs to improve readability.
Internally, the method uses the ast module to break down the equation and identify names/IDs to check and convert. However, the syntax of ast slightly differs in Python 3.7 and a name in an equation is typed as ast.Name (3.7) instead of ast.Constant (>3.7), which hasn't been looked after when initially implemented. Thus, the error persists exclusively in Python 3.7, which happens to be the base installation Colab and Binder use.
I was able to fix the error with my last commit and added some tests to make sure everything works as intended. I tested it in Colab and it works just fine now. Will do another test in Binder tomorrow.
jmrohwer
commented
2 years ago I can confirm that it works in Colab on my side and in Binder as well (after updating requirements.txt in Lauterbach2022 to point to main).
jmrohwer
commented
2 years ago I am re-opening the issue. Using the latest commit on main, the kinetic law is now not saved in Python versions 3.8-3.10, but in Python 3.7 everything is okay (so the other way round as originally). Moreover, when trying to save the enzymeml doc with .toFile(), the Python interpreter crashes and core dumps (on Py 3.8-3.10).
Could this have to do with the unit consistency checks introduced? I see also some species are now saved with quotes, does this maybe interfere with the MathML generation? It was not the case previously.
JR-1991
commented
2 years ago Thanks for pointing that out. The issue was with a logic in an if-pattern using regex pattern found in the setModel-method. It added two quotes instead of a single one what led to the error.
This is fixed now and has been tested with the notebook found in "Lauterbach_2022". It seems like the re package for 3.7 behaves different to the other ones. Also added a test to check if the MathML model has been added correctly to prevent such cases in the future.
jmrohwer
commented
2 years ago Thanks, all good now, tested on my side as well.
Execute the first part of
Scenario5/PySCeS/Cephalexin_Synthesis_Model4.ipynbfrom the Lauterbach2022 repo online (either via Binder, or via Google Colab), up until the archiveModel_4.omexis written (right before the section on Kinetic Modeling starts). In the resulting archive, the MathML is not written into the SBML file (experiment.xml), which causes the second part of the notebook to fail, when the same archive is read in again and then the simulators fail to find thekineticLawbecause it is not there.Below is a diff between the supplied file (from Model_4.omex, which is on the github repo), and the generated file. When I execute the notebook locally, the resultant file is correct and identical to the supplied file. This is from
mainon the PyEnzyme repo. I have no idea what is the cause of this, version differences between some package as it is available online and locally? But the error occurs with both Binder and Colab.No error messages are given during execution of the notebook.