Correct bugs when classifying variables per module

Author: enekomartinmartinez

docs/advanced_usage.rst

@@ -68,14 +68,17 @@ In a Vensim model with three separate views (e.g. `view_1`, `view_2` and `view_3
 | ├── _subscripts_many_views_model.json
 | ├── many_views_model.py
 
-
+The variables in each file will be sorted alphabetically, using their Python name.
 
 .. note ::
     Often, modelers wish to organise views further. To that end, a common practice is to include a particular character in the View name to indicate that what comes after it is the name of the subview. For instance, we could name one view as `ENERGY.Supply` and another one as `ENERGY.Demand`.
     In that particular case, setting the `subview_sep` kwarg equal to `["."]`, as in the code below, would name the translated views as `demand.py` and `supply.py` and place them inside the `ENERGY` folder::
 
       read_vensim("many_views_model.mdl", split_views=True, subview_sep=["."])
 
+.. note ::
+    If a variable appears as a `workbench variable` in more than one view, it will be added only to the module corresponding to the first view and a warning message will be printed. If a variable does not appear as a workbench variable in any view, it will be added to the main model file printing a warning message.
+
 If macros are present, they will be self-contained in files named after the macro itself. The macro inner variables will be placed inside the module that corresponds with the view in which they were defined.
 
 

docs/whats_new.rst

@@ -1,5 +1,34 @@
 What's New
 ==========
+v3.6.0 (2022/08/31)
+-------------------
+
+New Features
+~~~~~~~~~~~~
+- Include warning messages when a variable is defined in more than one view, when a control variable appears in a view or when a variable doesn't appear in any view as a `workbench variable` (:issue:`357`).
+- Force variables in a module to be saved alphabetically for being able to compare differences between versions (only for the models that are split by views).
+
+Breaking changes
+~~~~~~~~~~~~~~~~
+
+Deprecations
+~~~~~~~~~~~~
+
+Bug fixes
+~~~~~~~~~
+- Classify control variables in the main file always (:issue:`357`).
+
+Documentation
+~~~~~~~~~~~~~
+
+Performance
+~~~~~~~~~~~
+
+Internal Changes
+~~~~~~~~~~~~~~~~
+- Include :py:class:`pysd.translators.structures.abstract_model.AbstractControlElement` child of :py:class:`pysd.translators.structures.abstract_model.AbstractElement` to differentiate the control variables.
+
+
 v3.5.2 (2022/08/15)
 -------------------
 
@@ -14,7 +43,7 @@ Deprecations
 
 Bug fixes
 ~~~~~~~~~
-- Make sketch's _font_size_ optional.
+- Make sketch's `font_size` optional.
 
 Documentation
 ~~~~~~~~~~~~~

pysd/_version.py

@@ -1 +1 @@
-__version__ = "3.5.2"
+__version__ = "3.6.0"

pysd/builders/python/namespace.py

@@ -173,3 +173,24 @@ def make_python_identifier(self, string: str, prefix: str = None,
             self.cleanspace[clean_s] = identifier
 
         return identifier
+
+    def get_original_name(self, identifier):
+        """
+        Search for the original name of a variable's Python identifier.
+
+        Parameters
+        ----------
+        identifier: str
+            It should be a value in the namespace.
+
+        Rerturns
+        --------
+        original_name: str
+            The original name of the variable.
+
+        """
+        for key, value in self.namespace.items():
+            if value == identifier:
+                return key
+
+        raise ValueError(f"'{identifier}' not found in the namespace.")

pysd/builders/python/python_model_builder.py

@@ -5,14 +5,16 @@
 the user is only required to create a ModelBuilder object using the
 AbstractModel and call the `build_model` method.
 """
+from warnings import warn
 import textwrap
 import black
 import json
 from pathlib import Path
 from typing import Union
 
 from pysd.translators.structures.abstract_model import\
-    AbstractComponent, AbstractElement, AbstractModel, AbstractSection
+    AbstractComponent, AbstractElement, AbstractControlElement,\
+    AbstractModel, AbstractSection
 
 from . import python_expressions_builder as vs
 from .namespace import NamespaceManager
@@ -142,34 +144,88 @@ def _process_views_tree(self, view_name: str,
             view_content = {
                 self.namespace.cleanspace[var] for var in view_content
             }
-            # Get subview elements
-            subview_elems = [
-                element for element in self.elements_remaining
-                if element.identifier in view_content
+
+            # Get subview elements (ordered)
+            subview_elems = sorted(
+                [
+                    element for element in self.elements_remaining
+                    if element.identifier in view_content
+                    and not element.control_var
+                ],
+                key=lambda x: x.identifier)
+
+            # Get the names of the elements and include their
+            # information in the elements_added dictionary
+            subview_elems_names = [
+                element.identifier for element in subview_elems
             ]
+            view_path = ".".join(view_name.parts[1:])
+            self.elements_added.update({
+                var: view_path for var in subview_elems_names
+            })
+
+            if len(view_content) != len(subview_elems_names):
+                # Some elements from the view where not added
+                for var in view_content.difference(subview_elems_names):
+                    original_name = self.namespace.get_original_name(var)
+                    if var in self.elements_added:
+                        # Element already added in another view
+                        warn(
+                            f"Variable '{original_name}' is declared as "
+                            f"a workbench variable in '{view_path}' but "
+                            "it has been already added in "
+                            f"'{self.elements_added[var]}'."
+                        )
+                    else:
+                        # Element is a control variable
+                        warn(
+                            f"Control variable '{original_name}' is "
+                            "declared as a workbench variable in "
+                            f"'{view_path}'. As it is a control "
+                            "variable, this declaration will be ignored "
+                            "and added to the main module only."
+                        )
+
             # Remove elements from remaining ones
             [
                 self.elements_remaining.remove(element)
                 for element in subview_elems
             ]
-            # Build the module
-            self._build_separate_module(subview_elems, view_name, wdir)
-            return sorted(view_content)
+
+            if subview_elems:
+                # Build the module (only when they are variables)
+                self._build_separate_module(subview_elems, view_name, wdir)
+
+            return list(subview_elems_names)
         else:
             # The current view has subviews
-            wdir = wdir.joinpath(view_name)
-            wdir.mkdir(exist_ok=True)
-            return {
-                subview_name:
-                self._process_views_tree(subview_name, subview_content, wdir)
+            (wdir / view_name).mkdir(exist_ok=True)
+            subviews = {
+                subview_name: self._process_views_tree(
+                    view_name / subview_name, subview_content, wdir)
                 for subview_name, subview_content in view_content.items()
             }
+            # Avoid includying empty views to the dictionary
+            return {
+                subview_name: subview_content
+                for subview_name, subview_content in subviews.items()
+                if subview_content
+            }
 
     def _build_modular(self, elements_per_view: dict) -> None:
         """ Build modular section """
         self.elements_remaining = self.elements.copy()
+        self.elements_added = {}
         elements_per_view = self._process_views_tree(
-            "modules_" + self.model_name, elements_per_view, self.root)
+            Path("modules_" + self.model_name), elements_per_view, self.root)
+
+        for element in self.elements_remaining:
+            if not element.control_var:
+                warn(
+                    f"Variable '{element.name}' is not declared as a "
+                    "workbench variable in any view. It will be added to "
+                    "the main module."
+                )
         # Building main file using the build function
         self._build_main_module(self.elements_remaining)
 
@@ -211,15 +267,15 @@ def _build_separate_module(self, elements: list, module_name: str,
         Translated using PySD version %(version)s
         """
         ''' % {
-            "module_name": module_name,
+            "module_name": ".".join(module_name.parts[1:]),
             "version": __version__,
         })
         funcs = self._generate_functions(elements)
         text += funcs
         text = black.format_file_contents(
             text, fast=True, mode=black.FileMode())
 
-        outfile_name = module_dir.joinpath(module_name + ".py")
+        outfile_name = module_dir / module_name.with_suffix(".py")
 
         with outfile_name.open("w", encoding="UTF-8") as out:
             out.write(text)
@@ -478,6 +534,7 @@ class ElementBuilder:
     def __init__(self, abstract_element: AbstractElement,
                  section: SectionBuilder):
         self.__dict__ = abstract_element.__dict__.copy()
+        self.control_var = isinstance(abstract_element, AbstractControlElement)
         # Set element type and subtype to None
         self.type = None
         self.subtype = None

pysd/translators/structures/abstract_model.py

@@ -237,6 +237,37 @@ def _str_child(self, depth, indent) -> str:  # pragma: no cover
             ]).replace("\n", "\n" + indent)
 
 
+@dataclass
+class AbstractControlElement(AbstractElement):
+    """
+    Dataclass for a control element. This class is a child of
+    AbstractElement and has the same attributes.
+
+    Parameters
+    ----------
+    name: str
+        The name of the element.
+    components: list
+        The list of AbstractComponents that define this element.
+    units: str (optional)
+        The units of the element. '' by default.
+    limits: tuple (optional)
+        The limits of the element. (None, None) by default.
+    units: str (optional)
+        The documentation of the element. '' by default.
+
+    """
+    name: str
+    components: List[AbstractComponent]
+    units: str = ""
+    limits: tuple = (None, None)
+    documentation: str = ""
+
+    def __str__(self) -> str:  # pragma: no cover
+        return "AbstractControlElement:\t%s (%s, %s)\n%s\n" % (
+            self.name, self.units, self.limits, self.documentation)
+
+
 @dataclass
 class AbstractSubscriptRange:
     """

pysd/translators/vensim/vensim_section.py

@@ -10,7 +10,8 @@
 from pathlib import Path
 import parsimonious
 
-from ..structures.abstract_model import AbstractElement, AbstractSection
+from ..structures.abstract_model import\
+    AbstractElement, AbstractControlElement, AbstractSection
 
 from . import vensim_utils as vu
 from .vensim_element import Element, SubscriptRange, Component
@@ -160,15 +161,21 @@ def get_abstract_section(self) -> AbstractSection:
 
     def _merge_components(self) -> List[AbstractElement]:
         """Merge model components by their name."""
+        control_vars = ["initial_time", "final_time", "time_step", "saveper"]
         merged = {}
         for component in self.components:
             # get a safe name to merge (case and white/underscore sensitivity)
             name = component.name.lower().replace(" ", "_")
             if name not in merged:
                 # create new element if it is the first component
-                merged[name] = AbstractElement(
-                    name=component.name,
-                    components=[])
+                if name in control_vars:
+                    merged[name] = AbstractControlElement(
+                        name=component.name,
+                        components=[])
+                else:
+                    merged[name] = AbstractElement(
+                        name=component.name,
+                        components=[])
 
             if component.units:
                 # add units to element data

pysd/translators/xmile/xmile_element.py

@@ -20,7 +20,8 @@
 import numpy as np
 
 from ..structures.abstract_model import\
-    AbstractElement, AbstractLookup, AbstractComponent, AbstractSubscriptRange
+    AbstractElement, AbstractControlElement,\
+    AbstractLookup, AbstractComponent, AbstractSubscriptRange
 
 from ..structures.abstract_expressions import AbstractSyntax
 
@@ -467,11 +468,15 @@ def get_abstract_element(self) -> AbstractElement:
           with the Abstract Syntax Tree of the expression.
 
         """
-        ae = self._get_empty_abstract_element()
-        ae.components.append(AbstractComponent(
-            subscripts=([], []),
-            ast=self.ast))
-        return ae
+        return AbstractControlElement(
+            name=self.name,
+            units=self.units,
+            limits=self.limits,
+            documentation=self.documentation,
+            components=[
+                AbstractComponent(subscripts=([], []), ast=self.ast)
+            ]
+        )
 
 
 class SubscriptRange():