Skip to content

Conversation

rogersamso
Copy link
Contributor

This pull requests gives the user the option to get a translated model file organised by Vensim views. In the translated model, Vensim views can be seen as Python modules.

This option is controlled by the split_modules argument in the read_vensim function.

By default, the argument is set to False; hence, unless explicitly modified, the user gets the usual behaviour.

This new functionality has been added by adding a new grammar to parse the necessary bits from the Vensim sketch.

The option to split the model in views is particularly interesting for large models with tens of views. Translating those models into a single file may make the resulting Python model difficult to read and maintain.

In a Vensim model with three separate views (e.g. view_1, view_2 and view_3), setting split_modules to True would create the following tree inside the directory where the .mdl model is located:

| main-folder
| ├── modules_many_views_model
| │ ├── _modules.json
| │ ├── view_1.py
| │ ├── view_2.py
| │ └── view_3.py
| ├── _namespace_many_views_model.json
| ├── _subscripts_dict_many_views_model.json
| ├── many_views_model.py
|
|
If macros are present, they will be self-contained in files named as the macro itself. The macro inner variables will be placed inside the module that corresponds with the view in which they were defined.

@pep8speaks
Copy link

pep8speaks commented Jul 5, 2021

Hello @rogersamso! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 494:80: E501 line too long (117 > 79 characters)
Line 498:80: E501 line too long (115 > 79 characters)
Line 499:80: E501 line too long (252 > 79 characters)
Line 512:80: E501 line too long (190 > 79 characters)
Line 533:80: E501 line too long (156 > 79 characters)

Comment last updated at 2021-07-07 07:11:31 UTC

@coveralls
Copy link

coveralls commented Jul 5, 2021

Coverage Status

Coverage increased (+0.04%) to 98.543% when pulling ce3f359 on rogersamso:building_split_model_merge into 86b109f on JamesPHoughton:master.

@enekomartinmartinez
Copy link
Collaborator

Other changes included in this PR:

  • LICENSE added to MANIFEST as suggested by conda-forge maintainer
  • Added conda version badge to README and installation instructions to docs
  • Splitting option working with CLI as well
  • Add some tests for subscripts range parsing error

@rogersamso
Copy link
Contributor Author

Hello @rogersamso! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

* In the file [`pysd/py_backend/vensim/vensim2py.py`](https://github.com/JamesPHoughton/pysd/blob/cdf4499d16525df731895ff61c86f2af6114baf5/pysd/py_backend/vensim/vensim2py.py):

Line 503:80: E501 line too long (111 > 79 characters)
Line 507:80: E501 line too long (115 > 79 characters)
Line 508:80: E501 line too long (252 > 79 characters)
Line 521:80: E501 line too long (190 > 79 characters)
Line 542:80: E501 line too long (156 > 79 characters)

Comment last updated at 2021-07-05 12:12:28 UTC

Sorry about this. I think a good solution for it would be to move all grammars into a separate file.

Copy link
Collaborator

@JamesPHoughton JamesPHoughton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few quesetions - looks good otherwise. =) Nice formatting changes, that'll make it a lot easier to read.

Installing with conda
---------------------
To install PySD with conda into a conda environment, use the following command:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth mentioning that this doesn't use the standard conda channel, which is why we have the -c conda-forge. It's not important, but may save a few questions down the line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :)

def get_header(cls, outfile):
def get_header(cls, outfile, root=False):
"""
Returns the importing information to print in the model file
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the root argument here something that only takes a true/false value, or could it also take a path? looks like it's only used in the boolean sense, but if that's the long-term plan for it, might make sense to give it a different name?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update the name with force_root and add the documentation to the method. If this argument is True the _root is added in the model. If the parameter is false, the _root will be added only if the model has External objects.

--------
>>> get_file_sections(r'a~b~c| d~e~f| g~h~i|')
[{'returns': [], 'params': [], 'name': 'main', 'string': 'a~b~c| d~e~f| g~h~i|'}]
[{'returns': [], 'params': [], 'name': 'main', 'string': 'a~b~c| d~e~f|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does doctests handle this linebreak?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is not. I am recovering the previous version and improving some functions docs.

@rogersamso rogersamso closed this Jul 6, 2021
@rogersamso rogersamso reopened this Jul 6, 2021
@enekomartinmartinez enekomartinmartinez merged commit b8a6585 into SDXorg:master Jul 7, 2021
@rogersamso rogersamso deleted the building_split_model_merge branch April 26, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants