search

draeger-lab / ModelPolisher

ModelPolisher accesses the BiGG Models knowledgebase to annotate SBML models.
23 stars 7 forks source link

Update or deprecate matlab parser #37

Closed matthiaskoenig closed 5 years ago

matthiaskoenig commented 5 years ago

Currently, the matlab parser creates some issues with java updates. Also library is no longer maintained. The matlabparser com.jmatio is only used in cobra.COBRAparser.

There are two options to deal with this:

  1. update the parser with a current library https://github.com/HebiRobotics/MFL This is a nice and very local project, i.e., it is sufficient to update a single java file. Could be a great first step to improve the project by updating the code and adding tests for the matlab importer.

  2. A second option could be to deprecate the external parsers and focus on polishing SBML models. Other tools could provide the SBML conversion (would not require to maintain multiple converters in multiple tools). But has the risk of dependencies on other tools and not all java native. Because ModelPolisher is more of a command line tool this could work very well. E.g. use cobrapy to manage all the conversions via simple command line script.

    mat -> SBML3FBC
json -> SBML3FBC
SBML (old, legacy, cobra) -> SBML3FBC

    Focus ModelPolisher on polishing SBML models.

This solves part of #27 Best Matthias

draeger commented 5 years ago

As always, there are pros and cons to both options. Since the first option is indeed a decent exercise, I favor following this strategy for now. On the longer term, I'd propose to strip out that Matlab parser and to put it into a separate repository, which ModelPolisher can then load as a dependency.

matthiaskoenig commented 5 years ago

@draeger This is a great idea. I did not think about this. It would make a lot of sense to just put the sbml.io, i.e., the matlab and json parser in a separate repository. These could then be used by all the java tools, e.g., I could directly use this in cy3sbml ;) to import matlab and json models.

draeger commented 5 years ago

Indeed, the JSON parser could be moved elsewhere, too. But first, we should make sure it works because this change will bring a few further questions with it, namely how to structure this other repository... Let's open this box when the time is right.

At least, in this way, we will better follow the Unix principle: One tool for one task. Combinations of tools for complex tasks.

matthiaskoenig commented 5 years ago

I agree. So first fix of the parsers within the repository but with the clear perspective of putting in external repository. Fixing the matlab parser is a first very nice task with a clear scope.

codekaust commented 5 years ago

In the commit c48b81f570553e87295cd48a0934c8ca2db57162 (branch fix/updateMatlabParser), I have pushed the code containing catch for exception for struct field not present, csense field not present. The parser is working for e_coli_core.mat.

For further testing, I tried models AGORA Microbe Collection but it is giving exception in reading the file. As discussed in last meeting the issue lies with the library, we shall need to submit a PR there.

matthiaskoenig commented 5 years ago

@codekaust Could you add a unittest with the e_coli_core.mat model to the tests? What are the exceptions you get with the AGORA model?