Merge pull request #177 from moves-rwth/166-when-deleting-state-and-t…

…hen-adding-some-states-can-have-the-same-name

166 when deleting state and then adding some states can have the same name

stormvogel/model.py

@@ -92,7 +92,11 @@ def __init__(
         self.observation = None
 
         if name is None:
-            self.name = str(id)  # TODO Two states can have same name in some cases
+            if str(id) in used_names:
+                raise RuntimeError(
+                    "You need to choose a state name because of a conflict caused by removal of states."
+                )
+            self.name = str(id)
         else:
             self.name = name
 
@@ -740,7 +744,13 @@ def new_action(self, labels: frozenset[str] | str | None = None) -> Action:
         return action
 
     def reassign_ids(self):
-        """reassigns the ids of states, transitions and rates to be in order again"""
+        """reassigns the ids of states, transitions and rates to be in order again.
+        Mainly useful to keep consistent with storm."""
+
+        print(
+            "Warning: Using this can cause problems in your code if there are existing references to states by id."
+        )
+
         self.states = {
             new_id: value
             for new_id, (old_id, value) in enumerate(sorted(self.states.items()))
@@ -760,9 +770,10 @@ def reassign_ids(self):
             }
 
     def remove_state(
-        self, state: State, normalize: bool = True, reassign_ids: bool = True
+        self, state: State, normalize: bool = True, reassign_ids: bool = False
     ):
-        """properly removes a state, it can optionally normalize the model and reassign ids automatically"""
+        """Properly removes a state, it can optionally normalize the model and reassign ids automatically."""
+
         if state in self.states.values():
             # we remove the state from the transitions
             # first we remove transitions that go into the state

tests/test_model_methods.py

@@ -156,7 +156,7 @@ def test_normalize():
 def test_remove_state():
     # we make a normal ctmc and remove a state
     ctmc = examples.nuclear_fusion_ctmc.create_nuclear_fusion_ctmc()
-    ctmc.remove_state(ctmc.get_state_by_id(3))
+    ctmc.remove_state(ctmc.get_state_by_id(3), reassign_ids=True)
 
     # we make a ctmc with the state already missing
     new_ctmc = stormvogel.model.new_ctmc("Nuclear fusion")
@@ -194,7 +194,7 @@ def test_remove_state():
     mdp.set_transitions(mdp.get_initial_state(), transition)
 
     # we remove a state
-    mdp.remove_state(mdp.get_state_by_id(0))
+    mdp.remove_state(mdp.get_state_by_id(0), reassign_ids=True)
 
     # we make the mdp with the state already missing
     new_mdp = stormvogel.model.new_mdp(create_initial_state=False)
@@ -204,6 +204,27 @@ def test_remove_state():
 
     assert mdp == new_mdp
 
+    # this should fail:
+    new_dtmc = examples.die.create_die_dtmc()
+    state0 = new_dtmc.get_state_by_id(0)
+    new_dtmc.remove_state(new_dtmc.get_initial_state(), reassign_ids=True)
+    state1 = new_dtmc.get_state_by_id(0)
+
+    assert state0 != state1
+
+    # This should complain that names are the same:
+    try:
+        new_dtmc.new_state()
+        assert False
+    except RuntimeError:
+        pass
+
+    # But no longer if we do this:
+    try:
+        new_dtmc.new_state(name="new_name")
+    except RuntimeError:
+        assert False
+
 
 def test_remove_transitions_between_states():
     # we make a model and remove transitions between two states
@@ -322,7 +343,6 @@ def test_get_sub_model():
         [(1 / 6, new_dtmc.new_state(f"rolled{i}", {"rolled": i})) for i in range(2)]
     )
     new_dtmc.normalize()
-
     assert sub_model == new_dtmc