refactoring: minor changes and edited comments

unified_planning/io/pddl_writer.py

@@ -23,10 +23,6 @@
 
 import unified_planning as up
 import unified_planning.environment
-from unified_planning.model.action import Action
-from unified_planning.model.fnode import FNode
-from unified_planning.model.natural_transition import NaturalTransition
-from unified_planning.model.transition import Transition
 import unified_planning.model.walkers as walkers
 from unified_planning.model import (
     InstantaneousAction,
@@ -536,7 +532,7 @@ def _write_domain(self, out: IO[str]):
             self.problem.environment, self._get_mangled_name
         )
         costs: Dict[
-            Union[NaturalTransition, Action], Optional[FNode]
+            Union[up.model.NaturalTransition, up.model.Action], Optional[up.model.FNode]
         ] = {}  # TODO check if natural_transition should be here
         metrics = self.problem.quality_metrics
         if len(metrics) == 1:

unified_planning/model/natural_transition.py

@@ -12,11 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-"""
-This module defines the `Action` base class and some of his extensions.
-An `Action` has a `name`, a `list` of `Parameter`, a `list` of `preconditions`
-and a `list` of `effects`.
-"""
 
 
 import unified_planning as up
@@ -34,6 +29,14 @@
 
 from unified_planning.model.transition import Transition
 
+# TODO fix comments here
+# TODO check weather events/processes should have all the methods/other stuff that actions have. If yes, we need more tests.
+
+"""
+This module defines the `NaturalTransition` class and some of his extensions.
+An `NaturalTransition` has a `name`, a `list` of `Parameter`, a `list` of `preconditions`
+and a `list` of `effects`.
+"""
 
 """
 Below we have natural transitions. These are not controlled by the agent and would probably need a proper subclass. Natural transitions can be of two kinds:
@@ -283,40 +286,40 @@ def clone(self):
 
     @property
     def preconditions(self) -> List["up.model.fnode.FNode"]:
-        """Returns the `list` of the `Action` `preconditions`."""
+        """Returns the `list` of the `Event` `preconditions`."""
         return self._preconditions
 
     def clear_preconditions(self):
-        """Removes all the `Action preconditions`"""
+        """Removes all the `Event preconditions`"""
         self._preconditions = []
 
     @property
     def effects(self) -> List["up.model.effect.Effect"]:
-        """Returns the `list` of the `Action effects`."""
+        """Returns the `list` of the `Event effects`."""
         return self._effects
 
     def clear_effects(self):
-        """Removes all the `Action's effects`."""
+        """Removes all the `Event's effects`."""
         self._effects = []
         self._fluents_assigned = {}
         self._fluents_inc_dec = set()
         self._simulated_effect = None
 
     @property
     def conditional_effects(self) -> List["up.model.effect.Effect"]:
-        """Returns the `list` of the `action conditional effects`.
+        """Returns the `list` of the `event conditional effects`.
 
         IMPORTANT NOTE: this property does some computation, so it should be called as
         seldom as possible."""
         return [e for e in self._effects if e.is_conditional()]
 
     def is_conditional(self) -> bool:
-        """Returns `True` if the `action` has `conditional effects`, `False` otherwise."""
+        """Returns `True` if the `event` has `conditional effects`, `False` otherwise."""
         return any(e.is_conditional() for e in self._effects)
 
     @property
     def unconditional_effects(self) -> List["up.model.effect.Effect"]:
-        """Returns the `list` of the `action unconditional effects`.
+        """Returns the `list` of the `event unconditional effects`.
 
         IMPORTANT NOTE: this property does some computation, so it should be called as
         seldom as possible."""
@@ -332,9 +335,9 @@ def add_precondition(
         ],
     ):
         """
-        Adds the given expression to `action's preconditions`.
+        Adds the given expression to `event's preconditions`.
 
-        :param precondition: The expression that must be added to the `action's preconditions`.
+        :param precondition: The expression that must be added to the `event's preconditions`.
         """
         (precondition_exp,) = self._environment.expression_manager.auto_promote(
             precondition
@@ -360,7 +363,7 @@ def add_effect(
         forall: Iterable["up.model.variable.Variable"] = tuple(),
     ):
         """
-        Adds the given `assignment` to the `action's effects`.
+        Adds the given `assignment` to the `event's effects`.
 
         :param fluent: The `fluent` of which `value` is modified by the `assignment`.
         :param value: The `value` to assign to the given `fluent`.
@@ -383,7 +386,7 @@ def add_effect(
         if not fluent_exp.type.is_compatible(value_exp.type):
             # Value is not assignable to fluent (its type is not a subset of the fluent's type).
             raise UPTypeError(
-                f"InstantaneousAction effect has an incompatible value type. Fluent type: {fluent_exp.type} // Value type: {value_exp.type}"
+                f"Event effect has an incompatible value type. Fluent type: {fluent_exp.type} // Value type: {value_exp.type}"
             )
         self._add_effect_instance(
             up.model.effect.Effect(fluent_exp, value_exp, condition_exp, forall=forall)
@@ -397,7 +400,7 @@ def add_increase_effect(
         forall: Iterable["up.model.variable.Variable"] = tuple(),
     ):
         """
-        Adds the given `increase effect` to the `action's effects`.
+        Adds the given `increase effect` to the `event's effects`.
 
         :param fluent: The `fluent` which `value` is increased.
         :param value: The given `fluent` is incremented by the given `value`.
@@ -423,7 +426,7 @@ def add_increase_effect(
             raise UPTypeError("Effect condition is not a Boolean condition!")
         if not fluent_exp.type.is_compatible(value_exp.type):
             raise UPTypeError(
-                f"InstantaneousAction effect has an incompatible value type. Fluent type: {fluent_exp.type} // Value type: {value_exp.type}"
+                f"Event effect has an incompatible value type. Fluent type: {fluent_exp.type} // Value type: {value_exp.type}"
             )
         if not fluent_exp.type.is_int_type() and not fluent_exp.type.is_real_type():
             raise UPTypeError("Increase effects can be created only on numeric types!")
@@ -445,7 +448,7 @@ def add_decrease_effect(
         forall: Iterable["up.model.variable.Variable"] = tuple(),
     ):
         """
-        Adds the given `decrease effect` to the `action's effects`.
+        Adds the given `decrease effect` to the `event's effects`.
 
         :param fluent: The `fluent` which value is decreased.
         :param value: The given `fluent` is decremented by the given `value`.
@@ -467,7 +470,7 @@ def add_decrease_effect(
             raise UPTypeError("Effect condition is not a Boolean condition!")
         if not fluent_exp.type.is_compatible(value_exp.type):
             raise UPTypeError(
-                f"InstantaneousAction effect has an incompatible value type. Fluent type: {fluent_exp.type} // Value type: {value_exp.type}"
+                f"Event effect has an incompatible value type. Fluent type: {fluent_exp.type} // Value type: {value_exp.type}"
             )
         if not fluent_exp.type.is_int_type() and not fluent_exp.type.is_real_type():
             raise UPTypeError("Decrease effects can be created only on numeric types!")
@@ -484,39 +487,39 @@ def add_decrease_effect(
     def _add_effect_instance(self, effect: "up.model.effect.Effect"):
         assert (
             effect.environment == self._environment
-        ), "effect does not have the same environment of the action"
+        ), "effect does not have the same environment of the event"
         up.model.effect.check_conflicting_effects(
             effect,
             None,
             self._simulated_effect,
             self._fluents_assigned,
             self._fluents_inc_dec,
-            "action",
+            "event",
         )
         self._effects.append(effect)
 
     @property
     def simulated_effect(self) -> Optional["up.model.effect.SimulatedEffect"]:
-        """Returns the `action` `simulated effect`."""
+        """Returns the `event` `simulated effect`."""
         return self._simulated_effect
 
     def set_simulated_effect(self, simulated_effect: "up.model.effect.SimulatedEffect"):
         """
-        Sets the given `simulated effect` as the only `action's simulated effect`.
+        Sets the given `simulated effect` as the only `event's simulated effect`.
 
-        :param simulated_effect: The `SimulatedEffect` instance that must be set as this `action`'s only
+        :param simulated_effect: The `SimulatedEffect` instance that must be set as this `event`'s only
             `simulated effect`.
         """
         up.model.effect.check_conflicting_simulated_effects(
             simulated_effect,
             None,
             self._fluents_assigned,
             self._fluents_inc_dec,
-            "action",
+            "event",
         )
         if simulated_effect.environment != self.environment:
             raise UPUsageError(
-                "The added SimulatedEffect does not have the same environment of the Action"
+                "The added SimulatedEffect does not have the same environment of the Event"
             )
         self._simulated_effect = simulated_effect
 

unified_planning/model/transition.py

@@ -13,8 +13,8 @@
 # limitations under the License.
 #
 """
-This module defines the `Action` base class and some of his extensions.
-An `Action` has a `name`, a `list` of `Parameter`, a `list` of `preconditions`
+This module defines the `Transition` base class and some of his extensions.
+A `Transition` has a `name`, a `list` of `Parameter`, a `list` of `preconditions`
 and a `list` of `effects`.
 """
 
@@ -34,7 +34,7 @@
 
 
 class Transition(ABC):
-    """This is the `Action` interface."""
+    """This is the `Transition` interface."""
 
     def __init__(
         self,
@@ -53,15 +53,15 @@ def __init__(
             for n, t in _parameters.items():
                 assert self._environment.type_manager.has_type(
                     t
-                ), "type of parameter does not belong to the same environment of the action"
+                ), "type of parameter does not belong to the same environment of the transition"
                 self._parameters[n] = up.model.parameter.Parameter(
                     n, t, self._environment
                 )
         else:
             for n, t in kwargs.items():
                 assert self._environment.type_manager.has_type(
                     t
-                ), "type of parameter does not belong to the same environment of the action"
+                ), "type of parameter does not belong to the same environment of the transition"
                 self._parameters[n] = up.model.parameter.Parameter(
                     n, t, self._environment
                 )
@@ -92,63 +92,63 @@ def clone(self):
 
     @property
     def environment(self) -> Environment:
-        """Returns this `Action` `Environment`."""
+        """Returns this `Transition` `Environment`."""
         return self._environment
 
     @property
     def name(self) -> str:
-        """Returns the `Action` `name`."""
+        """Returns the `Transition `name`."""
         return self._name
 
     @name.setter
     def name(self, new_name: str):
-        """Sets the `Action` `name`."""
+        """Sets the `Transition` `name`."""
         self._name = new_name
 
     @property
     def parameters(self) -> List["up.model.parameter.Parameter"]:
-        """Returns the `list` of the `Action parameters`."""
+        """Returns the `list` of the `Transition parameters`."""
         return list(self._parameters.values())
 
     def parameter(self, name: str) -> "up.model.parameter.Parameter":
         """
-        Returns the `parameter` of the `Action` with the given `name`.
+        Returns the `parameter` of the `Transition` with the given `name`.
 
         Example
         -------
         >>> from unified_planning.shortcuts import *
         >>> location_type = UserType("Location")
         >>> move = InstantaneousAction("move", source=location_type, target=location_type)
-        >>> move.parameter("source")  # return the "source" parameter of the action, with type "Location"
+        >>> move.parameter("source")  # return the "source" parameter of the transition, with type "Location"
         Location source
         >>> move.parameter("target")
         Location target
 
-        If a parameter's name (1) does not conflict with an existing attribute of `Action` and (2) does not start with '_'
-        it can also be accessed as if it was an attribute of the action. For instance:
+        If a parameter's name (1) does not conflict with an existing attribute of `Transition` and (2) does not start with '_'
+        it can also be accessed as if it was an attribute of the transition. For instance:
 
         >>> move.source
         Location source
 
         :param name: The `name` of the target `parameter`.
-        :return: The `parameter` of the `Action` with the given `name`.
+        :return: The `parameter` of the `Transition` with the given `name`.
         """
         if name not in self._parameters:
-            raise ValueError(f"Action '{self.name}' has no parameter '{name}'")
+            raise ValueError(f"Transition '{self.name}' has no parameter '{name}'")
         return self._parameters[name]
 
     def __getattr__(self, parameter_name: str) -> "up.model.parameter.Parameter":
         if parameter_name.startswith("_"):
             # guard access as pickling relies on attribute error to be thrown even when
             # no attributes of the object have been set.
             # In this case accessing `self._name` or `self._parameters`, would re-invoke __getattr__
-            raise AttributeError(f"Action has no attribute '{parameter_name}'")
+            raise AttributeError(f"Transition has no attribute '{parameter_name}'")
         if parameter_name not in self._parameters:
             raise AttributeError(
-                f"Action '{self.name}' has no attribute or parameter '{parameter_name}'"
+                f"Transition '{self.name}' has no attribute or parameter '{parameter_name}'"
             )
         return self._parameters[parameter_name]
 
     def is_conditional(self) -> bool:
-        """Returns `True` if the `Action` has `conditional effects`, `False` otherwise."""
+        """Returns `True` if the `Transition` has `conditional effects`, `False` otherwise."""
         raise NotImplementedError

unified_planning/test/examples/processes.py

@@ -1,7 +1,8 @@
 from unified_planning.shortcuts import *
-from unified_planning.model.natural_transition import Process
 from unified_planning.test import TestCase
 
+# TODO we need more tests for better coverage
+
 
 def get_example_problems():
     problems = {}

unified_planning/test/test_pddl_io.py

@@ -442,30 +442,14 @@ def test_non_linear_car(self):
 
         self.assertTrue(problem is not None)
         self.assertEqual(len(problem.fluents), 8)
-        self.assertEqual(
-            len(
-                list(
-                    [
-                        ele
-                        for ele in problem.natural_transitions
-                        if isinstance(ele, Process)
-                    ]
-                )
-            ),
-            3,
+        n_proc = len(
+            list([el for el in problem.natural_transitions if isinstance(el, Process)])
         )
-        self.assertEqual(
-            len(
-                list(
-                    [
-                        ele
-                        for ele in problem.natural_transitions
-                        if isinstance(ele, Event)
-                    ]
-                )
-            ),
-            1,
+        n_eve = len(
+            list([el for el in problem.natural_transitions if isinstance(el, Event)])
         )
+        self.assertEqual(n_proc, 3)
+        self.assertEqual(n_eve, 1)
         found_drag_ahead = False
         for ele in problem.natural_transitions:
             if isinstance(ele, Process):