Repository: SDXorg/pysd

Issue:
Issues with subview-sep
When translating a model with views, subviews and subsubviews (hence two separators required) with split-views=True, and you get the order of the separators wrong in the CLI (i.e. ".,-" when it should be "-,."), the separation of views gets messed up. We could try to improve this, possibly by identifying what the most frequent pattern is in the names of the views. Maybe there's a simpler option...

In any case, you would expect that the warning message you actually get would help you realize the mistake you made. But the actual warning message (for each of the variables that cannot be placed in any of the modules) is:

_Variable 'var_name' is not declared as a workbench variable in any view. It will be added to the main module._

Which is actually not the case, and the problem is generated by PySD.

In addition, when running the translation from the CLI, if you pass this:

```bash
python -m pysd --translate my_model.mdl --split-views --subview-sep "-,."
```
You get this error:

```bash
PySD: error: argument --subview-sep: expected one argument
```

And to make it run properly, you actually need to add spaces in the subview-sep string, as follows:

```bash
python -m pysd --translate my_model.mdl --split-views --subview-sep " - , ."
```

This is not ideal, and should be fixed.

I might try to work on this, if you think it's worth it, @enekomartinmartinez. But I will not be able to work on it straightaway.




Discussion:

Patch:
diff --git a/docs/command_line_usage.rst b/docs/command_line_usage.rst
index b959ee8..ee1afbe 100644
--- a/docs/command_line_usage.rst
+++ b/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.
 
diff --git a/docs/development/adding_functions.rst b/docs/development/adding_functions.rst
index c947aed..a63d4da 100644
--- a/docs/development/adding_functions.rst
+++ b/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
 ---------------------------
diff --git a/docs/whats_new.rst b/docs/whats_new.rst
index 8f886db..4475515 100644
--- a/docs/whats_new.rst
+++ b/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>`_)
diff --git a/pysd/cli/main.py b/pysd/cli/main.py
index fb28940..e6e78f8 100644
--- a/pysd/cli/main.py
+++ b/pysd/cli/main.py
@@ -12,6 +12,7 @@ from pysd.translators.xmile.xmile_utils import supported_extensions as\
 
 from .parser import parser
 
+
 def main(args):
     """
     Main function. Reads user arguments, loads the models,
diff --git a/pysd/cli/parser.py b/pysd/cli/parser.py
index b22bb67..174630e 100644
--- a/pysd/cli/parser.py
+++ b/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 @@ parser.add_argument(
 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 @@ trans_arguments.add_argument(
 
 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 @@ parser.add_argument('model_file', metavar='model_file', type=check_model,
 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')
 
diff --git a/pysd/py_backend/allocation.py b/pysd/py_backend/allocation.py
index ef3e6b6..8127254 100644
--- a/pysd/py_backend/allocation.py
+++ b/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
diff --git a/pysd/translators/vensim/vensim_file.py b/pysd/translators/vensim/vensim_file.py
index 84488a8..2ac7d5f 100644
--- a/pysd/translators/vensim/vensim_file.py
+++ b/pysd/translators/vensim/vensim_file.py
@@ -216,7 +216,7 @@ class VensimFile():
                     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,16 +276,17 @@ class VensimFile():
             ).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
@@ -293,12 +294,23 @@ class VensimFile():
         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):


Test patch:
diff --git a/tests/pytest_pysd/pytest_allocation.py b/tests/pytest_pysd/pytest_allocation.py
index b749d33..3a22412 100644
--- a/tests/pytest_pysd/pytest_allocation.py
+++ b/tests/pytest_pysd/pytest_allocation.py
@@ -430,7 +430,7 @@ class TestAllocateByPriority():
                 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"]}),
@@ -438,7 +438,7 @@ class TestAllocateByPriority():
                 -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"]}),
@@ -446,8 +446,8 @@ class TestAllocateByPriority():
                 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\."
             ),
         ],
     )
diff --git a/tests/pytest_pysd/pytest_cli.py b/tests/pytest_pysd/pytest_cli.py
index 7770d24..4ca3901 100644
--- a/tests/pytest_pysd/pytest_cli.py
+++ b/tests/pytest_pysd/pytest_cli.py
@@ -12,6 +12,7 @@ import numpy as np
 
 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 @@ class TestPySD():
         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 @@ class TestPySD():
         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)
diff --git a/tests/pytest_pysd/pytest_select_submodel.py b/tests/pytest_pysd/pytest_select_submodel.py
index d768db1..470fa89 100644
--- a/tests/pytest_pysd/pytest_select_submodel.py
+++ b/tests/pytest_pysd/pytest_select_submodel.py
@@ -4,7 +4,9 @@ import shutil
 from pathlib import Path
 import numpy as np
 
+
 import pysd
+from pysd.translators.vensim.vensim_file import VensimFile
 
 
 @pytest.fixture(scope="class")
@@ -228,3 +230,76 @@ class TestGetVarsInModuleErrors:
         # assert raises are produced
         with pytest.raises(raise_type, match=error_message):
             model.get_vars_in_module(module)
+
+
+@pytest.mark.parametrize(
+    "views,expected,warns",
+    [
+        ([{"energy": {"energy_sub": {"energy_sub_sub1": {"var1", "var2"}}}},
+          {"energy": {"energy_sub": {"energy_sub_sub2": {"var3", "var4"}}}},
+          {"energy": {"energy_sub": {"var5", "var6"}}}
+          ],
+         {"energy": {"energy_sub": {"energy_sub_sub1": {"var1", "var2"},
+                                    "energy_sub_sub2": {"var3", "var4"},
+                                    "main": {"var5", "var6"}
+                                    }
+                     }
+          },
+         False
+         ),
+        ([{"energy": {"energy_sub": {"var5", "var6"}}},
+          {"energy": {"energy_sub": {"energy_sub_sub1": {"var1", "var2"}}}},
+          {"energy": {"energy_sub": {"energy_sub_sub2": {"var3", "var4"}}}},
+          ],
+         {"energy": {"energy_sub": {"main": {"var5", "var6"},
+                                    "energy_sub_sub1": {"var1", "var2"},
+                                    "energy_sub_sub2": {"var3", "var4"},
+
+                                    }
+                     }
+          },
+         False
+         ),
+        ([{"economy": {"varx", "vary"}},
+          {"energy": {"energy_sub": {"var5", "var6"}}},
+          {"energy": {"energy_sub": {"var7", "var8"}}},
+          {"energy": {"energy_sub": {"energy_sub_sub1": {"var1", "var2"}}}},
+          {"energy": {"energy_sub": {"energy_sub_sub2": {"var3", "var4"}}}},
+          ],
+         {"economy": {"varx", "vary"},
+          "energy": {"energy_sub": {"main": {"var5", "var6", "var7", "var8"},
+                                    "energy_sub_sub1": {"var1", "var2"},
+                                    "energy_sub_sub2": {"var3", "var4"},
+                                    }
+                     }
+          },
+         True
+         ),
+        ([{"energy": {"var1", "var2"}},
+          {"energy": {"energy_sub": {"energy_sub_sub": {"var3", "var4"}}}},
+          {"energy": {"energy_sub": {"var5", "var6"}}}
+          ],
+         {"energy": {"main": {"var1", "var2"},
+                     "energy_sub": {"energy_sub_sub": {"var3", "var4"},
+                                    "main": {"var5", "var6"}
+                                    }
+                     }
+          },
+         False
+         ),
+    ]
+)
+def test_merge_nested_dicts(views, expected, warns):
+
+    original_dict = {}
+    warning_msg = "Two views with same names but different separators where " \
+                  "identified. They will be joined in a single module"
+    if warns:
+        with pytest.warns(UserWarning, match=warning_msg):
+            for view in views:
+                VensimFile._merge_nested_dicts(original_dict, view)
+    else:
+        for view in views:
+            VensimFile._merge_nested_dicts(original_dict, view)
+
+    assert original_dict == expected