-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PySD v3.0 (Split parsing and building) #312
Conversation
@julienmalard I think you used to work with large subscripted models, it would be nice if you can use this branch to translate any of your models and let me know if you experience any bug or improvement, any help will be welcome :) |
@alexprey do you still use Xmile and Xmile translator or do you know someone who uses it or is interested? I may need some to split xmile translator from building and produce Xmile -> AbstractModel |
@JamesPHoughton you can start reviewing this PR if you want, if you have any doubt let me know, this way I can make the documentation more complete. |
Hello @enekomartinmartinez! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-05-20 11:55:09 UTC |
9d54d97
to
1cd08ba
Compare
@enekomartinmartinez there's a builder.py file in the translation folder. Is this intentional, or you forgot to remove it? |
@rogersamso , yes, this is intentional. It is the old builder, which was already placed there. It is still used for Xmile translation and for the old Vensim integration tests (which are used to ensure backwards compatibility). Once we have this PR ready the main idea is to remove this file and old translators (vemsim2py.py, xmile2py...) |
@@ -0,0 +1,276 @@ | |||
import re | |||
from pathlib import Path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All files inside the vensim folder start with vensim (e.g. vensim_element.py, vensim_file.py, etc.). This is a bit repetitive, I would just suppress the word vensim in front of all file names. I guess the same applies in the smile folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idem as previous, we need to see if we are comfortable with the current classes and once we are done find the best classification of files, including their names.
3c0d9eb
to
0de2adc
Compare
@JamesPHoughton The Xmile translator now works using the new builder via Abstract Model. Still need to document all the new scripts for both Vensim and Xmile translators. Note that a lot of features that were already included have never been tested, for example, there is no full integration test for a lookup with "extrapolate" or discrete. The same happens to different types of delays, smooths... |
0de2adc
to
4faeac4
Compare
…al-dev into split-parsing-building
@JamesPHoughton I added some new sections in the documentations for release notes and other pruposes, are you okay with all of these. |
The current version of PySD builds the models while parsing them with parsimonious. This has some inconveniences as we cannot build the model knowing its full structure. For example, when working with subscripted variables in the current version we cannot know which is the final subscripts of arithmetic expressions, which forces us to use some kind of magic functions such as 'rearrange', 'xrmerge' and '@subs'. Therefore, we cannot optimize the translated code and avoid xarray which is slowing the models with subscripts. Moreover, those improvements in the final model file (such as the dependencies dictionary) need to be done in three different areas (vensim parser, xmile parser and builder). Which force developers of one language to know also how work the parser of the other language. The main idea of splitting the parsing from the building is to enable several improvements and make PySD much easier to maintain. We would like to have something like:
Vensim model -> Intermediate Model (AbstractModel) -> PySD Model
Xmile model -> Intermediate Model (AbstractModel) -> PySD Model
this way we apply the builder over the Intermediate Model and all the improvements in the final model can be done by only change the builder from Intermediate to PySD. Moreover, this makes it easier to include new SD models as we only need to parse to AbstractModel and not include all the building, and enables the possibility in the future of including new output languages. See #302.
This PR changes completely how the models are translated. Vensim parsing and building are totally split and managed with objects. Now we have the following workflow:
Some other improvements added:
xrmerge
orrearrange
, this makes the subscripted models much faster.Note that I keep the old vensim2py to ensure the backwards compatibility of the library.
TODOs before merging:
Solves #310 #309 #306 #302 #301 #296 #116 #253 #168 #154 #107 #289 #168 #203
Future backwards compatible ideas:
Future no backwards compatible ideas: