Merge pull request #390 from rogersamso/subview_sep

Fix issues with the separation of Vensim views into modules and submodules

Author: enekomartinmartinez

docs/command_line_usage.rst

@@ -58,7 +58,15 @@ In order to split the Vensim model views in different files, as explained in :do
 
 .. code-block:: text
 
-    python -m pysd --split-views many_views_model.mdl
+    python -m pysd many_views_model.mdl --split-views
+
+The previous code will put each model view in a separate Python module. Additionally, if the names of the views include the concepts of subsubmodules (e.g., ENERGY-transformation.efficiency_improvement), the *--subview-sep* (subview separators) argument may be used to further classify the model equations:
+
+.. code-block:: text
+
+    python -m pysd many_views_and_subviews_model.mdl --split-views --subview-sep - .
+
+Note that passing any positional argument right after the *--subview-sep* argument will raise an error, so it is recommended to pass this argument as the last one.
 
 
 Outputting various run information
@@ -102,7 +110,7 @@ modified using the *-I/--initial_time*, *-F/--final-time*, *-T/--time-step* and
 
 .. code-block:: text
 
-    python -m pysd -I 2005 --final-time=2010 --time-step=1 Teacup.mdl
+    python -m pysd -I=2005 --final-time=2010 --time-step=1 Teacup.mdl
 
 will set the initial time to 2005, the final time to 2010 and the time step to 1.
 

docs/development/adding_functions.rst

@@ -1,6 +1,6 @@
 Adding new functions
 ====================
-In this section you may found some helpful examples for adding a new function to the PySD Python builder. Before starting adding any new feature or fuction, please, make sure that no one is working on it. Search if any open issue exists with the feature you want to work on or open a new one if it does not exist. Then, claim that you are working on it.
+In this section you may find some helpful examples for adding a new function to the PySD Python builder. Before starting adding any new feature or function, please, make sure that no one is working on it. Search if any open issue exists with the feature you want to work on or open a new one if it does not exist. Then, claim that you are working on it.
 
 Adding a hardcoded function
 ---------------------------

docs/whats_new.rst

@@ -13,14 +13,22 @@ Breaking changes
 Deprecations
 ~~~~~~~~~~~~
 
+Bug fixes
+~~~~~~~~~
+- Fix issue with the classification of variables in modules and submodules (:issue:`388`). When a model had a view with 3 sublevels (e.g. energy-transformation.losses) but another view was defined with only two of them (e.g. energy-transformation), the variables in the second view were placed in the main model file. Now, if this happens, the variables in the second view will be placed in a main.py file (i.e. energy/transformation/main.py). (`@rogersamso <https://github.com/rogersamso>`_)
+- Fix bug on the CLI when passing a hyphen as first value to the *--subview-sep* argument (:issue:`388`). (`@rogersamso <https://github.com/rogersamso>`_)
+- Fix bug on the CLI when parsing initial conditions (:issue:`395`). (`@rogersamso <https://github.com/rogersamso>`_)
+
 Documentation
 ~~~~~~~~~~~~~
+- The `Splitting Vensim views in different files` section in :doc:`command_line_usage` has been updated to include an example of the usage of the *--subview-sep* CLI argument. (`@rogersamso <https://github.com/rogersamso>`_)
 
 Performance
 ~~~~~~~~~~~
 
 Internal Changes
 ~~~~~~~~~~~~~~~~
+- The :py:meth:`_merge_nested_dicts` method from the :py:class:`pysd.translators.vensim.vensim_file.VensimFile` class has been made a static method, as it does not need to access any attribute of the instance, and it does facilitate unit testing. (`@rogersamso <https://github.com/rogersamso>`_)
 - The `pysd/translators/vensim/parsing_grammars/element_object.peg` grammar has been modified to be able to parse reality check elements. (`@rogersamso <https://github.com/rogersamso>`_)
 - :py:class:`pysd.translators.vensim.vensim_element.Constraint`  and :py:class:`pysd.translators.vensim.vensim_element.TestInputs` classes have been added, which inherit from the also newly created :py:class:`pysd.translators.vensim.vensim_element.GenericComponent`, which include the :py:meth:`parse` and :py:meth:`get_abstract_component` methods. (`@rogersamso <https://github.com/rogersamso>`_ and `@enekomartinmartinez <https://github.com/enekomartinmartinez>`_)
 - The :py:class:`pysd.translators.structures.abstract_model.AbstractSection` class now has two extra attributes (:py:data:`constraints` and :py:data:`input_tests`), which hold the :py:class:`pysd.translators.structures.abstract_model.AbstractConstraint` and :py:class:`pysd.translators.structures.abstract_model.AbstractTestInputs` objects. (`@rogersamso <https://github.com/rogersamso>`_)

pysd/cli/main.py

@@ -12,6 +12,7 @@
 
 from .parser import parser
 
+
 def main(args):
     """
     Main function. Reads user arguments, loads the models,

pysd/cli/parser.py

@@ -2,6 +2,7 @@
 cmdline parser
 """
 import os
+import re
 from ast import literal_eval
 import numpy as np
 import pandas as pd
@@ -113,15 +114,6 @@ def split_timestamps(string):
             f'See {docs} for examples.')
 
 
-def split_subview_sep(string):
-    """
-    Splits the subview separators
-    --subview-sep ' - ,.' -> [' - ', '.']
-
-    """
-    return string.split(",")
-
-
 def split_vars(string):
     """
     Splits the arguments from new_values.
@@ -141,8 +133,8 @@ def split_vars(string):
             var, value = string.split(':')
             type = 'initial'
 
-        if value.strip().isnumeric():
-            # value is float
+        if re.match(r"^[+-]?(\d*\.)?\d+$", value.strip()):
+            # value is a number
             return {var.strip(): (type, float(value))}
 
         # value is series
@@ -193,7 +185,7 @@ def __call__(self, parser, namespace, values, option_string=None):
 parser.add_argument(
     '-o', '--output-file', dest='output_file',
     type=check_output, metavar='FILE',
-    help='output file to save run outputs (.tab or .csv)')
+    help='output file to save run outputs (.tab, .csv or .nc)')
 
 parser.add_argument(
     '-p', '--progress', dest='progress',
@@ -282,11 +274,12 @@ def __call__(self, parser, namespace, values, option_string=None):
 
 trans_arguments.add_argument(
     '--subview-sep', dest='subview_sep',
-    action='store', type=split_subview_sep, default=[],
-    metavar='\'STRING1,STRING2,..,STRINGN\'',
-    help='further division of views split in subviews, by identifying the'
+    action='store', nargs="*", default=[],
+    metavar='separator_1 separator_2 ... separator_n',
+    help='further division of views into subviews, by identifying the '
          'separator string in the view name, only availabe if --split-views'
-         ' is used')
+         ' is used. Passing positional arguments after this argument will'
+         ' not work')
 
 
 #######################
@@ -316,17 +309,17 @@ def __call__(self, parser, namespace, values, option_string=None):
 parser.add_argument('new_values',
                     metavar='variable=new_value', type=split_vars,
                     nargs='*', action=SplitVarsAction,
-                    help='redefine the value of variable with new value.'
-                    'variable must be a model component, new_value can be a '
-                    'float or a a list of two list')
+                    help='redefine the value of variable with new value. '
+                    'variable must be a model component, new_value may be a '
+                    'float or a list of two lists')
 
-# The destionation new_values2 will never used as the previous argument
+# The destination new_values2 will never be used as the previous argument
 # is given also with nargs='*'. Nevertheless, the following variable
 # is declared for documentation
 parser.add_argument('new_values2',
                     metavar='variable:initial_value', type=split_vars,
                     nargs='*', action=SplitVarsAction,
-                    help='redefine the initial value of variable.'
+                    help='redefine the initial value of variable. '
                     'variable must be a model stateful element, initial_value'
                     ' must be a float')
 

pysd/py_backend/allocation.py

@@ -682,13 +682,11 @@ def allocate_by_priority(request, priority, width, supply):
 
     if np.any(width <= 0):
         raise ValueError(
-            f"width={width} is not allowed. width should be greater than 0."
-        )
+            f"width={width} \n is not allowed. width must be greater than 0.")
 
     if np.any(supply < 0):
         raise ValueError(
-            f"supply={supply} is not allowed. supply should be non-negative."
-        )
+            f"supply={supply} \n is not allowed. supply must not be negative.")
 
     if len(request.shape) == 1:
         # NUMPY: avoid '.values' and return directly the result of the

pysd/translators/vensim/vensim_file.py

@@ -216,7 +216,7 @@ def parse_sketch(self, subview_sep: List[str]) -> None:
                     nested_dict = {item: nested_dict}
                 # merging the new nested_dict into the views_dict, preserving
                 # repeated keys
-                self._merge_nested_dicts(views_dict, nested_dict)
+                VensimFile._merge_nested_dicts(views_dict, nested_dict)
         else:
             # view names do not have separators or separator characters
             # not provided
@@ -276,29 +276,41 @@ def _clean_file_names(*args):
             ).lstrip("0123456789")
             for name in args]
 
-    def _merge_nested_dicts(self, original_dict, dict_to_merge):
+    @staticmethod
+    def _merge_nested_dicts(original, to_merge):
         """
         Merge dictionaries recursively, preserving common keys.
 
         Parameters
         ----------
-        original_dict: dict
+        original: dict
             Dictionary onto which the merge is executed.
 
-        dict_to_merge: dict
+        to_merge: dict or set
             Dictionary to be merged to the original_dict.
 
         Returns
         -------
         None
 
         """
-        for key, value in dict_to_merge.items():
-            if (key in original_dict and isinstance(original_dict[key], dict)
-                    and isinstance(value, Mapping)):
-                self._merge_nested_dicts(original_dict[key], value)
+        if isinstance(to_merge, Mapping):
+            for key, value in to_merge.items():
+                if key in original:
+                    if isinstance(original[key], set):
+                        # convert it into a dict
+                        original[key] = {"main": original[key]}
+                    VensimFile._merge_nested_dicts(original[key], value)
+                else:
+                    original[key] = value
+        else:  # it's a set
+            if "main" not in original:
+                original["main"] = to_merge
             else:
-                original_dict[key] = value
+                original["main"].update(to_merge)
+                warnings.warn("Two views with same names but different "
+                              "separators where identified. They will be "
+                              "joined in a single module")
 
 
 class FileSectionsVisitor(parsimonious.NodeVisitor):

tests/pytest_pysd/pytest_allocation.py

@@ -430,24 +430,24 @@ def test__allocate_by_priority_1d(self, requests, priority, width,
                 0,
                 5,
                 ValueError,
-                r"width=0 is not allowed\. width should be greater than 0\."
+                r"width=0 \n is not allowed\. width must be greater than 0\."
             ),
             (  # negative width
                 xr.DataArray([6, 3, 3], {'dim': ["A", "B", "C"]}),
                 xr.DataArray([10, 5, 0], {'dim': ["A", "B", "C"]}),
                 -3,
                 7.5,
                 ValueError,
-                r"width=-3 is not allowed\. width should be greater than 0\."
+                r"width=-3 \n is not allowed\. width must be greater than 0\."
             ),
             (  # negative supply
                 xr.DataArray([6, 3, 3], {'dim': ["A", "B", "C"]}),
                 xr.DataArray([10, 5, 0], {'dim': ["A", "B", "C"]}),
                 3,
                 -7.5,
                 ValueError,
-                r"supply=-7\.5 is not allowed\. "
-                r"supply should be non-negative\."
+                r"supply=-7\.5 \n is not allowed\. "
+                r"supply must not be negative\."
             ),
         ],
     )

tests/pytest_pysd/pytest_cli.py

@@ -12,6 +12,7 @@
 
 import pysd
 from pysd.tools.benchmarking import load_outputs, assert_frames_close
+from pysd.cli.parser import parser
 
 test_model_ven = "test-models/samples/teacup/teacup.mdl"
 
@@ -254,6 +255,30 @@ def test_read_vensim_split_model_subviews(self, test_copy):
         assert (modules_dirname / "view_1" / "submodule_2.py").exists()
         assert (modules_dirname / "view_2.py").exists()
 
+    @pytest.mark.parametrize(
+        "model", ["more-tests/split_model/test_split_model_subviews.mdl"]
+    )
+    @pytest.mark.parametrize(
+        "separators,expected",
+        [
+            ([], []),
+            (['--subview-sep', '-'], ["-"]),
+            (['--subview-sep', '-', ","], ["-", ","]),
+            (['--subview-sep', '.', "-"], [".", "-"])
+
+        ])
+    def test_read_vensim_split_model_subviews_hyphen(self, test_copy,
+                                                     separators, expected):
+        """
+        Test that several arguments to the subview-sep option are correctly
+        parsed, and particularly when passing a hyphen.
+        """
+        mdl_path = str(test_copy)
+        args = [mdl_path, '--translate', '--split-views'] + separators
+
+        options = parser.parse_args(args)
+        options.subview_sep = expected
+
     @pytest.mark.parametrize("model", [test_model_ven])
     def test_run_return_timestamps(self, out_csv, out_tab, test_copy):
 
@@ -538,3 +563,71 @@ def test_save_without_name(self, out_tab, test_copy, tmp_path):
         out = load_outputs(tmp_path / outputs)
         out2 = pysd.read_vensim(test_copy).run()
         assert_frames_close(out, out2)
+
+
+@pytest.mark.parametrize(
+        "model", ["more-tests/split_model/test_split_model_sub_subviews.mdl"]
+    )
+class TestCLI():
+    """To test the CLI, ordering arguments differently"""
+
+    @pytest.mark.parametrize(
+        "cli_args,expected",
+        [(['-I=1.0', '--final-time=101',
+           '--split-views', '--subview-sep', '-', '.'],
+          {"new_values": {"param": {"another var": 2.0},
+                          "initial": {"Stock": -1.1}},
+           "initial_time": 1.0,
+           "final_time": 101,
+           "split_views": True,
+           "subview_sep": ["-", "."]
+           }
+          ),
+         (['--subview-sep', '-', '.', '-I=1.0', '--final-time=101',
+           '--split-views'],
+          {"new_values": {"param": {"another var": 2.0},
+                          "initial": {"Stock": -1.1}},
+           "initial_time": 1.0,
+           "final_time": 101,
+           "split_views": True,
+           "subview_sep": ["-", "."]
+           }
+          )
+         ],
+        ids=["subview_sep_last", "subview_sep_first"]
+        )
+    def test_cli_arguments_parsing(self, test_copy, cli_args, expected):
+
+        mdl_path = str(test_copy)
+
+        positional_args = [mdl_path, 'another var=2', 'Stock:-1.1']
+
+        # putting positional arguments at the begining
+        positionals_first = positional_args + cli_args
+        parsed = parser.parse_args(positionals_first)
+
+        for arg in expected:
+            assert expected[arg] == getattr(parsed, arg)
+
+    @pytest.mark.xfail(
+        reason="Passing positional arguments after nargs argument")
+    @pytest.mark.parametrize(
+        "cli_args,expected",
+        [(['--split-views', '--subview-sep', '-', '.'],
+          {"split_views": True, "subview_sep": ["-", "."]}
+          ),
+         (['another var=2', 'Stock:-1.1'],
+          {"new_values": {"param": {"another var": 2.0},
+                          "initial": {"Stock": -1.1}}}
+          )
+         ]
+    )
+    def test_positional_arguments_after_subview_sep(elf, test_copy, cli_args,
+                                                    expected):
+
+        mdl_path = str(test_copy)
+
+        # putting positional arguments after the subview_sep argument
+        positionals_last = cli_args + [mdl_path]
+
+        parsed = parser.parse_args(positionals_last)