Improper handling of reserved symbols in SBML import

[dweindl]

Using a parameter with ID k in an SBML model results in compiler errors. Did not yet investigate in detail, but the problem seems to be that _clean_reserved_symbols() does not replace reserved symbols everywhere:

No replacement of reserved symbols here:

amicimodel_p.h:52:11: error: conflicting declaration ‘const realtype** p’
   52 | #define k p[51]

Leading to various errors of this type:

Resulting in amicimodel_dwdx.cpp:17:110: note: in expansion of macro ‘k’
   17 | void dwdx_amicimodel(realtype *dwdx, const realtype t, const realtype *x, const realtype *p, const realtype *k, const realtype *h, const realtype *w, const realtype *tcl){
      |                                                                                                              ^
amicimodel_dwdx.cpp:17:91: note: previous declaration as ‘const realtype* p’
   17 | void dwdx_amicimodel(realtype *dwdx, const realtype t, const realtype *x, const realtype *p, const realtype *k, const realtype *h, const realtype *w, const realtype *tcl){
      |                                                                           ~~~~~~~~~~~~~~~~^
amicimodel_dwdx.cpp: In function ‘void amici::model_amicimodel::dwdx_amicimodel(amici::realtype*, amici::realtype, const realtype*, const realtype*, const realtype*, const realtype*, const realtype*)’:

Correctly replaced symbol here:

amicimodel_dwdx.cpp:18:65: error: ‘amici_k’ was not declared in this scope; did you mean ‘amici’?
   18 |     dflux_r2616 = amici_k;  // dwdx[0]
      |                                                                 ^~~~~~~
      |                                                                 amici

[dweindl]

When addressing that, enable -Werror for model builds. This will show additional errors, e.g. model entities named NULL colliding with other C++ entities. We might want to just prefix/suffix all model identifiers in order to avoid long lists of potentially problematic names. Any C++ type, keyword, function or variable name, preprocessor macro name, ... used in (the files included by) the model files is problematic.

[dweindl]

Having a compartment with ID x results in:

de_export.py:1034, in DEModel.import_from_sbml_importer.<locals>.transform_dxdt_to_concentration(species_id, dxdt)
   1028     raise SBMLException(
   1029         f"Species {species_id} is in a compartment {comp} that is"
   1030         f" defined by an algebraic equation. This is not"
   1031         f" supported."
   1032     )
   1033 else:
-> 1034     v = si.compartments[comp]
   1036     if v == 1.0:
   1037         return dxdt

KeyError: x

Some occurrences of x are correctly replaced by amici_x, others are not.

Ideally, this masking would already happen when we generate those symbols, so we don't have to substitute later.

EDIT: Possible approach:

• have dict of all reserved symbols mapping to their replacement, e.g. reserved = {'x': sp.Symbol('_amici_x'), ...}
• raise if any identifier already starts with our prefix (override dict.__getitem__)
• add function sanitize_id = lambda id: return reserved.get(id, id)
• add reserved to all sympify(locals=...)

[FFroehlich]

Having a compartment with ID x results in:

de_export.py:1034, in DEModel.import_from_sbml_importer.<locals>.transform_dxdt_to_concentration(species_id, dxdt)
   1028     raise SBMLException(
   1029         f"Species {species_id} is in a compartment {comp} that is"
   1030         f" defined by an algebraic equation. This is not"
   1031         f" supported."
   1032     )
   1033 else:
-> 1034     v = si.compartments[comp]
   1036     if v == 1.0:
   1037         return dxdt

KeyError: x

Some occurrences of x are correctly replaced by amici_x, others are not.

That‘s bad. This is implemented via ‚_replace_in_all_expressions‘ right. Might lead to other errors that aren‘t as evident.

Ideally, this masking would already happen when we generate those symbols, so we don't have to substitute later.

Yes.