Improper handling of reserved symbols in SBML import
Opened this issue · 3 comments
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
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.
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 allsympify(locals=...)
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 byamici_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.