climateinteractive / SDEverywhere

SDEverywhere translates System Dynamics models from Vensim to C, JavaScript, and WebAssembly
http://sdeverywhere.org/
MIT License
56 stars 21 forks source link

Translator to C is not support identifiers with apex symbol #6

Open alexprey opened 6 years ago

alexprey commented 6 years ago

Hi, @ToddFincannon! I found issue with model execution by our translator vensim models to C-code. I'm have following model: employees1-1.mdl.txt

And after running him, I got error in compilation time:

image


Regards, Alexey Mulyukin, Core lead-developer From sdCloud.io development team.

ToddFincannon commented 6 years ago

SDEverywhere has several issues that prevent it from translating this model. The first two are not simple to fix, so will be deferred for the time being. If it is important to have this model convert correctly, we need someone to contribute a C implementation of RANDOM NORMAL. The other issues can be worked around by renaming the variables.

alexprey commented 6 years ago

Transform the + character in variables like "Firing + Losing". This represents a larger unsolved problem: roundtripping Vensim and C variable names. We currently discard most special characters in Vensim names by replacing them with an underscore. This makes a nice, readable C variable name, but it is impossible to get the original Vensim variable name back.

In PySD solution in solves by introducing the namespaces map. When new vensim variable introduced into python code, at generates two additions in source code:

And this mapping used for model generating and accessing to proper components in original Vensim models.

Implementation of RANDOM NORMAL can be extracted from c source of NumPy math library or smth like it

travisfranck commented 6 years ago

On this third point β€” roundtripping is one issue (not important though?). Another issue is the potential for name collisions, if two vensim variable names get stripped down to the same C name. I don’t think roundtripping nor name collisions are a high priority though. Problems that will rarely come up, IMHO.

On Dec 19, 2017, at 11:49 PM, Todd Fincannon notifications@github.com wrote:

SDEverywhere has several issues that prevent it from translating this model. The first two are not simple to fix, so will be deferred for the time being. If it is important to have this model convert correctly, we need someone to contribute a C implementation of RANDOM NORMAL. The other issues can be worked around by renaming the variables.

Implement the RANDOM NORMAL Vensim function.

Update the parser to allow the apostrophe ' character in variable names, as seen in Little's Law Tenure.

Transform the + character in variables like "Firing + Losing". This represents a larger unsolved problem: roundtripping Vensim and C variable names. We currently discard most special characters in Vensim names by replacing them with an underscore. This makes a nice, readable C variable name, but it is impossible to get the original Vensim variable name back.

β€” You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ToddFincannon/SDEverywhere/issues/6#issuecomment-352971232, or mute the thread https://github.com/notifications/unsubscribe-auth/ALCXqJnzhCHK2Ok1oiHGCj8fucOkSLjKks5tCJ_wgaJpZM4RHAln.

alexprey commented 6 years ago

Name collision is not critical issue, it will easy fix with using unique number at the end of conveted name. I think it is not matter if we can find names like jelly_beams1 and jelly_beams2 that in original named as jelly beams and jelly beam's. But i think that this case is very strange and user should be aware with this names, because may be it just a typo error

JamesPHoughton commented 6 years ago

Texting with baby on my lap, sorry for typos...

I found the namespace issue to be nontrivial if you want something homan-readavle and also unique. I manage namespaces with the function

make_python_identifier(string, namespace=None, reserved_words=None, convert='drop', handle='force'):

Which is here: https://github.com/JamesPHoughton/pysd/blob/master/pysd/py_backend/utils.py

If you don't want to just translate every character one for one, and instead make some form of simplification, you have to keep track of what you've already translated... The simplified name conflicts seemed to come up in users models more than I would have thought, so I had to do this pretty early in dev.

πŸ™‚ James

On Wed, Dec 20, 2017 at 11:41 AM alexprey notifications@github.com wrote:

Name collision is not critical issue, it will easy fix with using unique number at the end of conveted name. I think it is not matter if we can find names like jelly_beams1 and jelly_beams2 that in original named as jelly beams and jelly beam's. But i think that this case is very strange and user should be aware with this names, because may be it just a typo error

β€” You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ToddFincannon/SDEverywhere/issues/6#issuecomment-353115250, or mute the thread https://github.com/notifications/unsubscribe-auth/AEGuXrUlWRlG322NSq_W6Je3toA3B_Ojks5tCTivgaJpZM4RHAln .