Assign / unassign sequence should be specific to sequence type

I previously incorrectly believed that sequence types are mutually exclusive. E.g. you can have max 1 relationship between two tasks. Now after looking at more schedules I realise it's logically allowed to have more than one sequence relationship. For example simultaneous SS + FF.

src/ifcopenshell-python/ifcopenshell/api/sequence/assign_sequence.py

@@ -106,8 +106,16 @@ def assign_sequence(
         # to be 2000-01-05.
         ifcopenshell.api.sequence.cascade_schedule(model, task=formwork)
     """
+    # Matched on the type as well as the pair. Two tasks may legitimately be
+    # sequenced more than once with different types — the classic case is a
+    # "ladder", where a start to start lets the follower begin once the leader
+    # has begun and a finish to finish stops it ending before the leader ends.
+    # Both constraints are real and neither implies the other, so matching on
+    # the pair alone silently hands back the wrong relationship: a caller adding
+    # a second sequence would instead be handed the first, and editing the type
+    # on it would destroy the constraint that was already there.
     for rel in related_process.IsSuccessorFrom or []:
-        if rel.RelatingProcess == relating_process:
+        if rel.RelatingProcess == relating_process and rel.SequenceType == sequence_type:
             return rel
     rel = file.create_entity(
         "IfcRelSequence",

src/ifcopenshell-python/ifcopenshell/api/sequence/duplicate_task.py

@@ -156,6 +156,7 @@ class Usecase:
                             self.file,
                             relating_process=relating_process,
                             related_process=related_process,
+                            sequence_type=inverse.SequenceType,
                         )
                         if inverse.TimeLag:
                             ifcopenshell.api.sequence.assign_lag_time(

src/ifcopenshell-python/ifcopenshell/api/sequence/unassign_sequence.py

@@ -19,17 +19,28 @@
 import ifcopenshell
 import ifcopenshell.api.sequence
 import ifcopenshell.util.element
+from typing import Optional
 
 
 def unassign_sequence(
     file: ifcopenshell.file,
     relating_process: ifcopenshell.entity_instance,
     related_process: ifcopenshell.entity_instance,
+    sequence_type: Optional[str] = None,
 ) -> None:
     """Removes a sequence relationship between tasks
 
+    Two tasks may be sequenced more than once with different types — see
+    :func:`ifcopenshell.api.sequence.assign_sequence` — so removing "the"
+    relationship between a pair is ambiguous. Left unspecified, every sequence
+    between the two is removed, which is what "make them unrelated" means and
+    what this did before the parameter existed. Name a type to remove only that
+    one and leave any others in place.
+
     :param relating_process: The previous / predecessor task.
     :param related_process: The next / successor task.
+    :param sequence_type: Optionally, remove only the sequence of this type.
+        Choose from FINISH_START, FINISH_FINISH, START_START, or START_FINISH.
     :return: None
 
     Example:
@@ -58,9 +69,12 @@ def unassign_sequence(
             relating_process=zone1, related_process=zone2)
     """
     for rel in related_process.IsSuccessorFrom or []:
-        if rel.RelatingProcess == relating_process:
+        if rel.RelatingProcess != relating_process:
-            history = rel.OwnerHistory
+            continue
-            file.remove(rel)
+        if sequence_type is not None and rel.SequenceType != sequence_type:
-            if history:
+            continue
-                ifcopenshell.util.element.remove_deep2(file, history)
+        history = rel.OwnerHistory
+        file.remove(rel)
+        if history:
+            ifcopenshell.util.element.remove_deep2(file, history)
     ifcopenshell.api.sequence.cascade_schedule(file, task=related_process)

src/ifcopenshell-python/test/api/sequence/test_assign_sequence.py

@@ -0,0 +1,95 @@
+# IfcOpenShell - IFC toolkit and geometry engine
+# Copyright (C) 2021 Dion Moult <dion@thinkmoult.com>
+#
+# This file is part of IfcOpenShell.
+#
+# IfcOpenShell is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# IfcOpenShell is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with IfcOpenShell.  If not, see <http://www.gnu.org/licenses/>.
+
+import ifcopenshell.api.sequence
+import test.bootstrap
+
+
+# NOTE: sequence module features rely on entities introduced in IFC4
+# therefore no IFC2X3 tests
+class TestAssignSequence(test.bootstrap.IFC4):
+    def test_assigning_a_sequence(self):
+        predecessor = ifcopenshell.api.sequence.add_task(self.file)
+        successor = ifcopenshell.api.sequence.add_task(self.file)
+        rel = ifcopenshell.api.sequence.assign_sequence(
+            self.file, relating_process=predecessor, related_process=successor
+        )
+        assert rel.is_a("IfcRelSequence")
+        assert rel.RelatingProcess == predecessor
+        assert rel.RelatedProcess == successor
+        assert rel.SequenceType == "FINISH_START"
+
+    def test_assigning_a_sequence_of_a_chosen_type(self):
+        predecessor = ifcopenshell.api.sequence.add_task(self.file)
+        successor = ifcopenshell.api.sequence.add_task(self.file)
+        rel = ifcopenshell.api.sequence.assign_sequence(
+            self.file,
+            relating_process=predecessor,
+            related_process=successor,
+            sequence_type="START_START",
+        )
+        assert rel.SequenceType == "START_START"
+
+    def test_not_assigning_the_same_sequence_twice(self):
+        predecessor = ifcopenshell.api.sequence.add_task(self.file)
+        successor = ifcopenshell.api.sequence.add_task(self.file)
+        rel1 = ifcopenshell.api.sequence.assign_sequence(
+            self.file, relating_process=predecessor, related_process=successor
+        )
+        rel2 = ifcopenshell.api.sequence.assign_sequence(
+            self.file, relating_process=predecessor, related_process=successor
+        )
+        assert rel1 == rel2
+        assert len(self.file.by_type("IfcRelSequence")) == 1
+
+    def test_assigning_two_sequences_of_different_types_to_the_same_pair(self):
+        # A "ladder": the follower may start once the leader has started, and
+        # may not finish before the leader finishes. Both constraints are real
+        # and neither implies the other, so both relationships must survive.
+        predecessor = ifcopenshell.api.sequence.add_task(self.file)
+        successor = ifcopenshell.api.sequence.add_task(self.file)
+        start = ifcopenshell.api.sequence.assign_sequence(
+            self.file,
+            relating_process=predecessor,
+            related_process=successor,
+            sequence_type="START_START",
+        )
+        finish = ifcopenshell.api.sequence.assign_sequence(
+            self.file,
+            relating_process=predecessor,
+            related_process=successor,
+            sequence_type="FINISH_FINISH",
+        )
+        assert start != finish
+        assert len(self.file.by_type("IfcRelSequence")) == 2
+        assert {rel.SequenceType for rel in successor.IsSuccessorFrom} == {
+            "START_START",
+            "FINISH_FINISH",
+        }
+
+    def test_not_confusing_the_two_directions_of_a_pair(self):
+        task1 = ifcopenshell.api.sequence.add_task(self.file)
+        task2 = ifcopenshell.api.sequence.add_task(self.file)
+        forwards = ifcopenshell.api.sequence.assign_sequence(
+            self.file, relating_process=task1, related_process=task2
+        )
+        backwards = ifcopenshell.api.sequence.assign_sequence(
+            self.file, relating_process=task2, related_process=task1
+        )
+        assert forwards != backwards
+        assert len(self.file.by_type("IfcRelSequence")) == 2

src/ifcopenshell-python/test/api/sequence/test_unassign_sequence.py

@@ -0,0 +1,111 @@
+# IfcOpenShell - IFC toolkit and geometry engine
+# Copyright (C) 2021 Dion Moult <dion@thinkmoult.com>
+#
+# This file is part of IfcOpenShell.
+#
+# IfcOpenShell is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# IfcOpenShell is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with IfcOpenShell.  If not, see <http://www.gnu.org/licenses/>.
+
+import ifcopenshell.api.sequence
+import test.bootstrap
+
+
+# NOTE: sequence module features rely on entities introduced in IFC4
+# therefore no IFC2X3 tests
+class TestUnassignSequence(test.bootstrap.IFC4):
+    def _pair(self):
+        return (
+            ifcopenshell.api.sequence.add_task(self.file),
+            ifcopenshell.api.sequence.add_task(self.file),
+        )
+
+    def test_unassigning_a_sequence(self):
+        predecessor, successor = self._pair()
+        ifcopenshell.api.sequence.assign_sequence(
+            self.file, relating_process=predecessor, related_process=successor
+        )
+        ifcopenshell.api.sequence.unassign_sequence(
+            self.file, relating_process=predecessor, related_process=successor
+        )
+        assert len(self.file.by_type("IfcRelSequence")) == 0
+
+    def test_doing_nothing_if_the_tasks_are_not_sequenced(self):
+        predecessor, successor = self._pair()
+        ifcopenshell.api.sequence.unassign_sequence(
+            self.file, relating_process=predecessor, related_process=successor
+        )
+        assert len(self.file.by_type("IfcRelSequence")) == 0
+
+    def test_unassigning_every_sequence_between_the_pair_by_default(self):
+        predecessor, successor = self._pair()
+        for sequence_type in ("START_START", "FINISH_FINISH"):
+            ifcopenshell.api.sequence.assign_sequence(
+                self.file,
+                relating_process=predecessor,
+                related_process=successor,
+                sequence_type=sequence_type,
+            )
+        ifcopenshell.api.sequence.unassign_sequence(
+            self.file, relating_process=predecessor, related_process=successor
+        )
+        assert len(self.file.by_type("IfcRelSequence")) == 0
+
+    def test_unassigning_only_the_named_type(self):
+        predecessor, successor = self._pair()
+        for sequence_type in ("START_START", "FINISH_FINISH"):
+            ifcopenshell.api.sequence.assign_sequence(
+                self.file,
+                relating_process=predecessor,
+                related_process=successor,
+                sequence_type=sequence_type,
+            )
+        ifcopenshell.api.sequence.unassign_sequence(
+            self.file,
+            relating_process=predecessor,
+            related_process=successor,
+            sequence_type="START_START",
+        )
+        rels = self.file.by_type("IfcRelSequence")
+        assert len(rels) == 1
+        assert rels[0].SequenceType == "FINISH_FINISH"
+
+    def test_not_unassigning_a_type_that_is_not_there(self):
+        predecessor, successor = self._pair()
+        ifcopenshell.api.sequence.assign_sequence(
+            self.file,
+            relating_process=predecessor,
+            related_process=successor,
+            sequence_type="START_START",
+        )
+        ifcopenshell.api.sequence.unassign_sequence(
+            self.file,
+            relating_process=predecessor,
+            related_process=successor,
+            sequence_type="FINISH_FINISH",
+        )
+        assert len(self.file.by_type("IfcRelSequence")) == 1
+
+    def test_leaving_the_other_direction_alone(self):
+        task1, task2 = self._pair()
+        ifcopenshell.api.sequence.assign_sequence(
+            self.file, relating_process=task1, related_process=task2
+        )
+        ifcopenshell.api.sequence.assign_sequence(
+            self.file, relating_process=task2, related_process=task1
+        )
+        ifcopenshell.api.sequence.unassign_sequence(
+            self.file, relating_process=task1, related_process=task2
+        )
+        rels = self.file.by_type("IfcRelSequence")
+        assert len(rels) == 1
+        assert rels[0].RelatingProcess == task2