fix: disable adding prefix 'M_' and 'R_' in exportModel function

[edkerk]

Main improvements in this PR:

Duplicate of #356, reverting the revert by #359 :) (see explanation there).

The changes made in PR are based on the discussion and consensus in #353

• stops adding prefixes M_, R_, and C_ to metabolite, reaction, and compartment ids in exportModel function (RAVEN doesn't correct but report errors)
• introduces regular expressions to checkModelStruct for checking whether the IDs are in compliance with libSBML specification
• fully runs checkModelStruct by default in exportModel and reports instances of invalid ids as errors (prevent their spreading to the community)

I hereby confirm that I have:

• Tested my code on my own machine
• Followed the development guidelines.
• Selected devel as a target branch
• If needed, asked first in the Gitter chat room about this PR

[edkerk]

Line 118-132 should be removed, no longer required as invalid IDs would already have thrown an error.

RAVEN/io/exportModel.m

Lines 118 to 132 in a459d6c

	%Convert ids to SBML-convenient format. This is to avoid the data loss when
	%unsupported characters are included in ids. Here we are using part from
	%convertSBMLID, originating from the COBRA Toolbox
	model.rxns=regexprep(model.rxns,'([^0-9_a-zA-Z])','__${num2str($1+0)}__');
	model.mets=regexprep(model.mets,'([^0-9_a-zA-Z])','__${num2str($1+0)}__');
	model.comps=regexprep(model.comps,'([^0-9_a-zA-Z])','__${num2str($1+0)}__');
	if isfield(model,'genes')
	problemGenes=find(~cellfun('isempty',regexp(model.genes,'([^0-9_a-zA-Z])')));
	originalGenes=model.genes(problemGenes);
	replacedGenes=regexprep(model.genes(problemGenes),'([^0-9_a-zA-Z])','__${num2str($1+0)}__');
	model.genes(problemGenes)=replacedGenes;
	for i=1:numel(problemGenes)
	model.grRules = regexprep(model.grRules, ['(^|\s|\()' originalGenes{i} '($|\s|\))'], ['$1' replacedGenes{i} '$2']);
	end
	end

[haowang-bioinfo]

agree

[mihai-sysbio]

no longer required as invalid IDs would already have thrown an error.

@edkerk this got me thinking - are invalid reported throwing errors for all import formats, including importExcelModel?

[edkerk]

no longer required as invalid IDs would already have thrown an error.

@edkerk this got me thinking - are invalid reported throwing errors for all import formats, including importExcelModel?

@mihai-sysbio What I meant is that exportModel will throw the error via checkModelStruct. importModel would not need to call checkModelStruct, as libSBML would have failed to load a model with illegal characters. For importExcelModel though, and a future readYaml function (which at this time does not exist in RAVEN 2, but is planned to be included) it indeed would make sense to call checkModelStruct.

[edkerk]

Should not be merged before #364 is merged and pushed from devel to main. Release 2.5.4 has then minor changes, while release 2.6.0 (this PR) will have the significant change that I/O is no longer resulting in the same model in MATLAB as in previous versions, as it no longer uses the COBRA-style prefixes.

[mihai-sysbio]

Resolving the conflict and the merging of #364 brings this PR in a merge-able state.

Release 2.5.4 has then minor changes, while release 2.6.0 (this PR) will have the significant change that I/O is no longer resulting in the same model in MATLAB as in previous versions, as it no longer uses the COBRA-style prefixes.

I guess we then expect one more PR that releases 2.5.4. before merging this one though @edkerk ?

[edkerk]

I'm still hesitant with this, as it can break backwards compatibility with people's scripts. It changes the identifier representation (e.g. M_glc6p instead of glc6p) when changing RAVEN version. Might it not be so drastic that it should (part of a) major release?

edit: I moved my further discussion to issue #353.

[simas232]

I tried to test this PR with the iAL1006 model from tutorials. Due to the suggested changes in this PR we have a new problem - there are many rxns and mets that begin with a numeric char so the model is no longer compliant with SBML specifications and the exportModel function therefore fails. I am not sure how common it is for metabolite and reaction IDs to start with a number, but this is definitely problematic for GEMs that use BiGG IDs for mets and/or rxns.

[edkerk]

I agree @simas232, this should be properly resolved, also discussed in #353. To avoid confusion, I'll change this PR to draft for now.

[mihai-sysbio]

Good catch @simas232.

this is definitely problematic for GEMs that use BiGG IDs for mets and/or rxns.

To me, this sounds very much like a problem with the GEMs themselves not being SBML-compatible. The main question who's responsibility it is to address it.

I could imagine a version of the tutorial that starts with a Colab notebook with cobrapy that imports and exports the model, just to obtain a valid SBML (assuming cobrapy also implements this prefixing functionality). Alternatively, such a "patched" GEM could be delivered with the tutorial, as long as this is documented.