add sbml infix parsing and printing #1785

lkeegan · 2021-05-05T11:29:23Z

add parser function parse_sbml()
add print function sbml()
add sbml_tokenizer.*, sbml_parser.* to symengine/parser/sbml (see design question: multiple parsers #1780)
use api.prefix {sbml} in sbml_parser.yy to avoid nameclashes with main parser
sbml_parser.yy is based on Simplify the parser #1596
sbml infix format is described here
apologies for the large PR!

- add parser function `parse_sbml()` - add print function `sbml()` - add sbml_tokenizer, sbml_parser, etc to symengine/parser/sbml - use `api.prefix {sbml}` in sbml_parser.yy to avoid nameclashes with main parser

rikardn · 2021-05-05T11:54:42Z

I am excited about this PR! I don't think it is related, but we need to do something about the fail of the test_tutorials. It is always nice to have everything in green before reviewing PRs.

symengine/parser/sbml/sbml_parser.cpp

isuruf · 2021-05-05T20:14:41Z

symengine/parser/sbml/sbml_parser.cpp

+                     + "' has no arguments");
+}
+
+RCP<const Basic> SbmlParser::functionify(const std::string &name,


We can reduce the code duplication here using the same trick as init_sbml_printer_names. What do you think?

I tried this here: a159dcd

Is that what you meant? If so, it does reduce duplication, but it might also require more maintenance - e.g. if a new function is added to the main parser that is not supported by sbml, it would also have to be explicitly removed from the sbml parser. Either way is fine by me though.

Having to remember to remove something in one end when something was added in another sounds like an extra maintenance burden. Don't we want to avoid that?

Yes, looks good to me.

if a new function is added to the main parser that is not supported by sbml, it would also have to be explicitly removed from the sbml parser

Is it necessary to remove them though? Would it be bad if they were kept?

Yes, looks good to me.

if a new function is added to the main parser that is not supported by sbml, it would also have to be explicitly removed from the sbml parser

Is it necessary to remove them though? Would it be bad if they were kept?

it could mean invalid sbml infix gets parsed as valid

~~but it would give an error when printing: so that avoids the invalid sbml from being exported somewhere else~~

it would also print without errors, since sbml inherits the default function names, but the resulting sbml infix wouldn't be valid

if a function is added to the main parser with the same name as an existing sbml one it would take precedence (since insert is a no-op if the key already exists), which could potentially change the meaning of this function in the sbml parser

but this would then cause a sbml_parser test to fail

tldr; no, it's probably fine to keep them

I guess the question is what should the sbml parser/printer do with math that the main parser+symengine understands, but that is not part of sbml?

Raise an error

need to define all valid functions -> code duplication

or inherit default functions & remove invalid ones -> maintenance burden when defaults change

Fallback to the default printer/parser behaviour

no code duplication or maintenance issues

Currently we do 1. when parsing sbml, and 2. when printing sbml

As a user I would prefer option 1. for both printing & parsing: raising an error is much more helpful than silently parsing/producing invalid infix

But the current setup is also fine, since the user can just re-parse the printed output to check it is valid

As a maintainer I would probably prefer to implement this by defining all valid functions (i.e. duplication rather than maintenance) but either way is fine by me

What do you think?

I also think the parser should throw on illegal sbml. Also the printer should throw for things that cannot be supported or isn't currenty supported in sbml. Some cases I guess could be supported with workarounds if needed. For example we have a printer for Fortran code in Pharmpy that converts sign(x) to ABS(X) / X (if x is known to be positive) or similar.

@isuruf what are your thoughts on this?

I'm okay with either.

ok, I'll keep the current behaviour for now then (sbml parser throws on invalid sbml, sbml printer falls back to default printer), and leave improving the printer to throw on invalid sbml for a future PR.

I've reverted to defining all sbml parser functions to avoid having to update the sbml parser when new functions are added to the main parser (4054db5)

isuruf · 2021-05-05T20:15:48Z

symengine/parser/sbml/sbml_parser.h

+namespace SymEngine
+{
+
+class SbmlParser


Can you make this inherit from the string parse so that parse_numeric and other functions need not be duplicated?

Yes - after struggling with strange compiler errors it turns out this also required a change to the main parser: b9d40f8

symengine/printers/sbml.cpp

- bison uses `YYSTYPE` internally, and defines a macro to refer to the actual type, e.g. `#define YYSTYPE ParserSType` - when including multiple parsers this macro expansion resulted in missing/re- definition errors

…d of defining all functions

isuruf · 2021-05-06T19:26:08Z

symengine/parser/sbml/sbml_parser.cpp

+    functions.insert({"arcsin", asin});
+    functions.insert({"arccos", acos});
+    functions.insert({"arctan", atan});
+    functions.insert({"arcsec", asec});
+    functions.insert({"arccsc", acsc});
+    functions.insert({"arccot", acot});
+    functions.insert({"arcsinh", asinh});
+    functions.insert({"arccosh", acosh});
+    functions.insert({"arctanh", atanh});
+    functions.insert({"arcsech", asech});
+    functions.insert({"arccoth", acoth});
+    functions.insert({"arccsch", acsch});


I'm fine with adding these into the main parser.

done here: 8b80e66

…fault parser functions

lkeegan · 2021-06-07T09:17:03Z

thanks @rikardn and @isuruf for reviewing! I think this is ready to merge if ok with you?

rikardn · 2021-06-07T10:09:17Z

I think the code is looking good. Would it be possible to keep the code coverage above 90% ?

lkeegan · 2021-06-08T07:47:50Z

@rikardn I added a couple more tests (and found #1804 in the process), but I don't think the coverage will get much higher: nearly all the written code is fully covered - the remaining uncovered code is in the source files auto-generated by re2c/bison & appears to be mostly error handling or implementing things that we don't use: https://app.codecov.io/gh/symengine/symengine/compare/master...sbml_parser/diff

add sbml infix parsing and printing

06a4e32

- add parser function `parse_sbml()` - add print function `sbml()` - add sbml_tokenizer, sbml_parser, etc to symengine/parser/sbml - use `api.prefix {sbml}` in sbml_parser.yy to avoid nameclashes with main parser

rikardn reviewed May 5, 2021

View reviewed changes

symengine/parser/sbml/sbml_parser.cpp Outdated Show resolved Hide resolved

isuruf reviewed May 5, 2021

View reviewed changes

rikardn reviewed May 5, 2021

View reviewed changes

symengine/printers/sbml.cpp Show resolved Hide resolved

lkeegan added 5 commits May 6, 2021 11:07

rename SymEngine::YYSTYPE to SymEngine::ParserSType

b9d40f8

- bison uses `YYSTYPE` internally, and defines a macro to refer to the actual type, e.g. `#define YYSTYPE ParserSType` - when including multiple parsers this macro expansion resulted in missing/re- definition errors

remove sbml_parser_stype: use ParserSType instead

e16c540

make SbmlTokenizer inherit from Tokenizer

57f5695

make SbmlParser inherit from Parser

e435142

add init_parser_single_arg_functions, SbmlParser modifies this instea…

a159dcd

…d of defining all functions

lkeegan mentioned this pull request May 6, 2021

sbml infix parser status #1786

Open

6 tasks

isuruf reviewed May 6, 2021

View reviewed changes

add 'arc*' versions of inverse trig functions to parser

8b80e66

rikardn mentioned this pull request May 14, 2021

design question: multiple parsers #1780

Closed

lkeegan added 3 commits May 16, 2021 14:40

Merge branch 'master' into sbml_parser

381b1ea

Merge branch 'master' into sbml_parser

5da33a5

define all sbml parser functions instead of inheriting & modifying de…

4054db5

…fault parser functions

lkeegan added 3 commits June 7, 2021 20:58

Merge branch 'master' into sbml_parser

8d87951

update sbml parser to use bison cpp API

3f7c2c1

add sbml parser tests

87de2e2

lkeegan added 3 commits June 8, 2021 16:20

remove unused & operator from sbml_tokenizer.re

8675361

remove convert_xor option from sbml parser (a^x means pow(a,x))

94b3f06

remove unused @ token from grammar

20c91f3

rikardn approved these changes Jun 8, 2021

View reviewed changes

lkeegan merged commit 24c9d19 into symengine:master Jun 10, 2021

lkeegan mentioned this pull request Jun 10, 2021

use symengine sbml parser/printer spatial-model-editor/spatial-model-editor#569

Closed

lkeegan mentioned this pull request Jun 11, 2021

support all libsbml math everywhere spatial-model-editor/spatial-model-editor#473

Closed

lkeegan deleted the sbml_parser branch June 12, 2021 06:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add sbml infix parsing and printing #1785

add sbml infix parsing and printing #1785

lkeegan commented May 5, 2021

rikardn commented May 5, 2021

isuruf May 5, 2021

lkeegan May 6, 2021

rikardn May 6, 2021

isuruf May 6, 2021

lkeegan May 6, 2021 •

edited

lkeegan May 7, 2021

rikardn May 7, 2021

lkeegan Jun 1, 2021

isuruf Jun 2, 2021

lkeegan Jun 7, 2021

isuruf May 5, 2021

lkeegan May 6, 2021

isuruf May 6, 2021

lkeegan May 7, 2021

lkeegan commented Jun 7, 2021

rikardn commented Jun 7, 2021

lkeegan commented Jun 8, 2021

add sbml infix parsing and printing #1785

add sbml infix parsing and printing #1785

Conversation

lkeegan commented May 5, 2021

rikardn commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkeegan May 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkeegan commented Jun 7, 2021

rikardn commented Jun 7, 2021

lkeegan commented Jun 8, 2021

lkeegan May 6, 2021 •

edited