From c5cb346677bb2be0de17d0434231ad59f8517597 Mon Sep 17 00:00:00 2001
From: francescalb <francesca.l.bleken@sintef.no>
Date: Thu, 16 Mar 2023 15:32:56 +0100
Subject: [PATCH 1/5] get_ancestors and get_descendants have the same
 arguments.

They now have the same arguments:
classes, generations and common.wq
---
 ontopy/graph.py                 | 12 +++++--
 ontopy/ontology.py              | 58 +++++++++++++++++++--------------
 tests/test_generation_search.py | 55 +++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+), 27 deletions(-)

diff --git a/ontopy/graph.py b/ontopy/graph.py
index 008955120..739cf6144 100644
--- a/ontopy/graph.py
+++ b/ontopy/graph.py
@@ -400,9 +400,17 @@ def add_branch(  # pylint: disable=too-many-arguments,too-many-locals
             nodeattrs=nodeattrs,
             **attrs,
         )
-
+        common_ancestors = False
+        ancestor_generations = None
+        if include_parents in ("common", "closest"):
+            common_ancestors = True
+        elif isinstance(include_parents, int):
+            ancestor_generations = include_parents
         parents = self.ontology.get_ancestors(
-            classes, include=include_parents, strict=True
+            classes,
+            common=common_ancestors,
+            generations=ancestor_generations,
+            strict=True,
         )
         if parents:
             for parent in parents:
diff --git a/ontopy/ontology.py b/ontopy/ontology.py
index a7f35262d..a79e3e219 100644
--- a/ontopy/ontology.py
+++ b/ontopy/ontology.py
@@ -4,7 +4,7 @@
 If desirable some of these additions may be moved back into owlready2.
 """
 # pylint: disable=too-many-lines,fixme,arguments-differ,protected-access
-from typing import TYPE_CHECKING, Optional, Union, Sequence
+from typing import TYPE_CHECKING, Optional, Union
 import os
 import itertools
 import inspect
@@ -1468,18 +1468,25 @@ def closest_common_ancestor(*classes):
             "A closest common ancestor should always exist !"
         )
 
-    def get_ancestors(self, classes, include="all", strict=True):
+    def get_ancestors(
+        self,
+        classes: "Union[List, ThingClass]",
+        common: bool = False,
+        generations: int = None,
+        strict: bool = True,
+    ) -> set:
         """Return ancestors of all classes in `classes`.
-        classes to be provided as list
-
-        The values of `include` may be:
-          - None: ignore this argument
-          - "all": Include all ancestors.
-          - "closest": Include all ancestors up to the closest common
-            ancestor of all classes.
-          - int: Include this number of ancestor levels.  Here `include`
-            may be an integer or a string that can be converted to int.
+        Args:
+            classes: class(es) for which ancestors are desired.
+            common: whether to only return the closest common ancestor.
+            generations: Include this number of generations, default is all.
+            strict: only return real ancestors if True.
+        Returns:
+            A set of ancestors for given number of generations.
         """
+        if not isinstance(classes, Iterable):
+            classes = [classes]
+
         ancestors = set()
         if not classes:
             return ancestors
@@ -1490,22 +1497,23 @@ def addancestors(entity, counter, subject):
                     subject.add(parent)
                     addancestors(parent, counter - 1, subject)
 
-        if isinstance(include, str) and include.isdigit():
-            include = int(include)
-
-        if include == "all":
-            ancestors.update(*(_.ancestors() for _ in classes))
-        elif include == "closest":
-            closest = self.closest_common_ancestor(*classes)
+        if sum(map(bool, [common, generations])) > 1:
+            raise ValueError(
+                "Only one of `generations` or `common` may be specified."
+            )
+        if common:
+            closest_ancestor = self.closest_common_ancestor(*classes)
             for cls in classes:
                 ancestors.update(
-                    _ for _ in cls.ancestors() if closest in _.ancestors()
+                    _
+                    for _ in cls.ancestors()
+                    if closest_ancestor in _.ancestors()
                 )
-        elif isinstance(include, int):
+        elif isinstance(generations, int):
             for entity in classes:
-                addancestors(entity, int(include), ancestors)
-        elif include not in (None, "None", "none", ""):
-            raise ValueError('include must be "all", "closest" or None')
+                addancestors(entity, generations, ancestors)
+        else:
+            ancestors.update(*(_.ancestors() for _ in classes))
 
         if strict:
             return ancestors.difference(classes)
@@ -1519,7 +1527,7 @@ def get_descendants(
     ) -> set:
         """Return descendants/subclasses of all classes in `classes`.
         Args:
-            classes: to be provided as list.
+            classes: class(es) for which descendants are desired.
             common: whether to only return descendants common to all classes.
             generations: Include this number of generations, default is all.
         Returns:
@@ -1529,7 +1537,7 @@ def get_descendants(
             'generations' defaults to all.
         """
 
-        if not isinstance(classes, Sequence):
+        if not isinstance(classes, Iterable):
             classes = [classes]
 
         descendants = {name: [] for name in classes}
diff --git a/tests/test_generation_search.py b/tests/test_generation_search.py
index b24028072..793d8288c 100755
--- a/tests/test_generation_search.py
+++ b/tests/test_generation_search.py
@@ -80,3 +80,58 @@ def test_descendants(emmo: "Ontology", repo_dir: "Path") -> None:
     assert onto.get_descendants([onto.Tree, onto.NaturalDye], common=True) == {
         onto.Avocado
     }
+
+
+def test_ancestors(emmo: "Ontology", repo_dir: "Path") -> None:
+    from ontopy import get_ontology
+    from ontopy.utils import LabelDefinitionError
+
+    ontopath = repo_dir / "tests" / "testonto" / "testontology.ttl"
+
+    onto = get_ontology(ontopath).load()
+
+    # Test that default gives all ancestors.
+    assert onto.get_ancestors(onto.NorwaySpruce) == {
+        onto.Spruce,
+        onto.Tree,
+        onto.EvergreenTree,
+        onto.Thing,
+    }
+
+    # Test that asking for 0 generations returns empty set
+    assert onto.get_ancestors(onto.NorwaySpruce, generations=0) == set()
+
+    # Check that number of generations are returned correctly
+    assert onto.get_ancestors(onto.NorwaySpruce, generations=2) == {
+        onto.Spruce,
+        onto.EvergreenTree,
+    }
+
+    assert onto.get_ancestors(onto.NorwaySpruce, generations=1) == {
+        onto.Spruce,
+    }
+    # Check that no error is generated if one of the classes do
+    # not have enough parents for all given generations
+    assert onto.get_ancestors(onto.NorwaySpruce, generations=10) == (
+        onto.get_ancestors(onto.NorwaySpruce)
+    )
+
+    # Check that ancestors of a list is returned correctly
+    assert onto.get_ancestors([onto.NorwaySpruce, onto.Avocado]) == {
+        onto.Tree,
+        onto.EvergreenTree,
+        onto.Spruce,
+        onto.NaturalDye,
+        onto.Thing,
+    }
+    # Check that classes up to closest common ancestor are returned
+
+    assert onto.get_ancestors(
+        [onto.NorwaySpruce, onto.Avocado], common=True
+    ) == {
+        onto.EvergreenTree,
+        onto.Spruce,
+    }
+
+    with pytest.raises(ValueError):
+        onto.get_ancestors(onto.NorwaySpruce, common=True, generations=4)

From b9582db1ec65842817e1976c19770752d0c91e17 Mon Sep 17 00:00:00 2001
From: francescalb <francesca.l.bleken@sintef.no>
Date: Thu, 16 Mar 2023 15:43:38 +0100
Subject: [PATCH 2/5] Added test for strict=False

---
 tests/test_generation_search.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/test_generation_search.py b/tests/test_generation_search.py
index 793d8288c..8dd7cd116 100755
--- a/tests/test_generation_search.py
+++ b/tests/test_generation_search.py
@@ -135,3 +135,15 @@ def test_ancestors(emmo: "Ontology", repo_dir: "Path") -> None:
 
     with pytest.raises(ValueError):
         onto.get_ancestors(onto.NorwaySpruce, common=True, generations=4)
+
+    # Test strict == False
+    assert onto.get_ancestors(
+        [onto.NorwaySpruce, onto.Avocado],
+        common=True,
+        strict=False,
+    ) == {
+        onto.EvergreenTree,
+        onto.Spruce,
+        onto.NorwaySpruce,
+        onto.Avocado,
+    }

From 5cb71464fd7cd8a167b1db19dbd4f07c098bb1d1 Mon Sep 17 00:00:00 2001
From: francescalb <francesca.l.bleken@sintef.no>
Date: Thu, 16 Mar 2023 20:48:12 +0100
Subject: [PATCH 3/5] Changed to argument as closest in get_ancestors.

---
 ontopy/graph.py                 |  8 ++++----
 ontopy/ontology.py              | 20 +++++++++++---------
 tests/test_generation_search.py |  6 +++---
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/ontopy/graph.py b/ontopy/graph.py
index 739cf6144..2bc1210d3 100644
--- a/ontopy/graph.py
+++ b/ontopy/graph.py
@@ -400,15 +400,15 @@ def add_branch(  # pylint: disable=too-many-arguments,too-many-locals
             nodeattrs=nodeattrs,
             **attrs,
         )
-        common_ancestors = False
+        closest_ancestors = False
         ancestor_generations = None
-        if include_parents in ("common", "closest"):
-            common_ancestors = True
+        if include_parents == "closest":
+            closest_ancestors = True
         elif isinstance(include_parents, int):
             ancestor_generations = include_parents
         parents = self.ontology.get_ancestors(
             classes,
-            common=common_ancestors,
+            closest=closest_ancestors,
             generations=ancestor_generations,
             strict=True,
         )
diff --git a/ontopy/ontology.py b/ontopy/ontology.py
index a79e3e219..13c2cc4da 100644
--- a/ontopy/ontology.py
+++ b/ontopy/ontology.py
@@ -1471,18 +1471,20 @@ def closest_common_ancestor(*classes):
     def get_ancestors(
         self,
         classes: "Union[List, ThingClass]",
-        common: bool = False,
+        closest: bool = False,
         generations: int = None,
         strict: bool = True,
     ) -> set:
         """Return ancestors of all classes in `classes`.
         Args:
-            classes: class(es) for which ancestors are desired.
-            common: whether to only return the closest common ancestor.
+            classes: class(es) for which ancestors should be returned.
             generations: Include this number of generations, default is all.
-            strict: only return real ancestors if True.
+            closest: If True, return all ancestors up to and including the
+                closest common ancestor. Return all if False.
+            strict: If True returns only real ancestors, i.e. `classes` are
+                are not included in the returned set.
         Returns:
-            A set of ancestors for given number of generations.
+            Set of ancestors to `classes`.
         """
         if not isinstance(classes, Iterable):
             classes = [classes]
@@ -1497,11 +1499,11 @@ def addancestors(entity, counter, subject):
                     subject.add(parent)
                     addancestors(parent, counter - 1, subject)
 
-        if sum(map(bool, [common, generations])) > 1:
+        if sum(map(bool, [closest, generations])) > 1:
             raise ValueError(
-                "Only one of `generations` or `common` may be specified."
+                "Only one of `generations` or `closest` may be specified."
             )
-        if common:
+        if closest:
             closest_ancestor = self.closest_common_ancestor(*classes)
             for cls in classes:
                 ancestors.update(
@@ -1522,8 +1524,8 @@ def addancestors(entity, counter, subject):
     def get_descendants(
         self,
         classes: "Union[List, ThingClass]",
-        common: bool = False,
         generations: int = None,
+        common: bool = False,
     ) -> set:
         """Return descendants/subclasses of all classes in `classes`.
         Args:
diff --git a/tests/test_generation_search.py b/tests/test_generation_search.py
index 8dd7cd116..3afe2c17a 100755
--- a/tests/test_generation_search.py
+++ b/tests/test_generation_search.py
@@ -127,19 +127,19 @@ def test_ancestors(emmo: "Ontology", repo_dir: "Path") -> None:
     # Check that classes up to closest common ancestor are returned
 
     assert onto.get_ancestors(
-        [onto.NorwaySpruce, onto.Avocado], common=True
+        [onto.NorwaySpruce, onto.Avocado], closest=True
     ) == {
         onto.EvergreenTree,
         onto.Spruce,
     }
 
     with pytest.raises(ValueError):
-        onto.get_ancestors(onto.NorwaySpruce, common=True, generations=4)
+        onto.get_ancestors(onto.NorwaySpruce, closest=True, generations=4)
 
     # Test strict == False
     assert onto.get_ancestors(
         [onto.NorwaySpruce, onto.Avocado],
-        common=True,
+        closest=True,
         strict=False,
     ) == {
         onto.EvergreenTree,

From acf7ac7a01d91ab35a0614f1781697eb2f08f82e Mon Sep 17 00:00:00 2001
From: francescalb <francesca.l.bleken@sintef.no>
Date: Tue, 11 Apr 2023 21:03:39 +0200
Subject: [PATCH 4/5] Some testing to keep temporarily.

---
 ontopy/graph.py                  |  2 +-
 tests/ontopy_tests/test_graph.py | 35 +++++++++++++++++++++++++++-----
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/ontopy/graph.py b/ontopy/graph.py
index 2bc1210d3..efbde1ef6 100644
--- a/ontopy/graph.py
+++ b/ontopy/graph.py
@@ -371,7 +371,7 @@ def add_branch(  # pylint: disable=too-many-arguments,too-many-locals
         also included."""
         if leaves is None:
             leaves = ()
-
+        print(leaves)
         classes = self.ontology.get_branch(
             root=root,
             leaves=leaves,
diff --git a/tests/ontopy_tests/test_graph.py b/tests/ontopy_tests/test_graph.py
index dbfc55891..7943e161d 100644
--- a/tests/ontopy_tests/test_graph.py
+++ b/tests/ontopy_tests/test_graph.py
@@ -77,7 +77,7 @@ class hasPartRenamed(owlready2.ObjectProperty):
     graph.save(tmpdir / "testonto.png")
 
 
-def test_emmo_graphs(emmo: "Ontology", tmpdir: "Path") -> None:
+def test_emmo_graphs(emmo: "Ontology", repo_dir: "Path") -> None:
     """Testing OntoGraph on various aspects of EMMO.
 
     Parameters:
@@ -90,7 +90,7 @@ def test_emmo_graphs(emmo: "Ontology", tmpdir: "Path") -> None:
     from ontopy.graph import OntoGraph, plot_modules
     from ontopy import get_ontology
     import warnings
-
+    tmpdir=repo_dir / "tmp"
     with warnings.catch_warnings():
         warnings.simplefilter("error")
         graph = OntoGraph(
@@ -217,8 +217,8 @@ def test_emmo_graphs(emmo: "Ontology", tmpdir: "Path") -> None:
         graph = OntoGraph(emmo)
         graph.add_entities(semiotic, relations="all", edgelabels=False)
         graph.add_legend()
-        graph.save(tmpdir / "measurement.png")
-
+        graph.save(tmpdir / "measurement.png", fmt="graphviz")
+        print('reductionistc')
         # Reductionistic perspective
         graph = OntoGraph(
             emmo,
@@ -236,7 +236,31 @@ def test_emmo_graphs(emmo: "Ontology", tmpdir: "Path") -> None:
             edgelabels=None,
         )
         graph.add_legend()
-        graph.save(tmpdir / "Reductionistic.png", fmt="graphviz")
+        graph.save(tmpdir / "Reductionistic.png")
+
+        print('add branch')
+        # Reductionistic perspective, choose leaf_generations
+        graph = OntoGraph(
+            emmo,
+            emmo.Reductionistic,
+            relations="all",
+            addnodes=False,
+            parents=2,
+            edgelabels=None,
+        )
+        graph.add_branch(emmo.Reductionistic,
+            leaves=[
+                emmo.Quantity,
+                emmo.String,
+                emmo.PrefixedUnit,
+                emmo.SymbolicConstruct,
+                emmo.Matter,
+            ],
+
+            )
+            
+        graph.add_legend()
+        graph.save(tmpdir / "Reductionistic_addbranch.png")
 
         # View modules
 
@@ -244,3 +268,4 @@ def test_emmo_graphs(emmo: "Ontology", tmpdir: "Path") -> None:
             "https://raw.githubusercontent.com/emmo-repo/EMMO/1.0.0-beta4/emmo.ttl"
         ).load()
         plot_modules(onto, tmpdir / "modules.png", ignore_redundant=True)
+ 

From 7e1e2c053f14ecfe5bf60112cdc994091a00abac Mon Sep 17 00:00:00 2001
From: francescalb <francesca.l.bleken@sintef.no>
Date: Tue, 2 May 2023 12:31:20 +0200
Subject: [PATCH 5/5] Added test as required by reviewer

---
 ontopy/graph.py                  |  1 -
 ontopy/ontology.py               | 17 ++++++-----
 tests/ontopy_tests/test_graph.py | 52 +++++++++++++++++++++++++++-----
 3 files changed, 53 insertions(+), 17 deletions(-)

diff --git a/ontopy/graph.py b/ontopy/graph.py
index efbde1ef6..b3f77958b 100644
--- a/ontopy/graph.py
+++ b/ontopy/graph.py
@@ -371,7 +371,6 @@ def add_branch(  # pylint: disable=too-many-arguments,too-many-locals
         also included."""
         if leaves is None:
             leaves = ()
-        print(leaves)
         classes = self.ontology.get_branch(
             root=root,
             leaves=leaves,
diff --git a/ontopy/ontology.py b/ontopy/ontology.py
index 75246f1a4..04b50712f 100644
--- a/ontopy/ontology.py
+++ b/ontopy/ontology.py
@@ -1534,23 +1534,24 @@ def addancestors(entity, counter, subject):
                     subject.add(parent)
                     addancestors(parent, counter - 1, subject)
 
-        if sum(map(bool, [closest, generations])) > 1:
-            raise ValueError(
-                "Only one of `generations` or `closest` may be specified."
-            )
         if closest:
+            if generations is not None:
+                raise ValueError(
+                    "Only one of `generations` or `closest` may be specified."
+                )
+
             closest_ancestor = self.closest_common_ancestor(*classes)
             for cls in classes:
                 ancestors.update(
-                    _
-                    for _ in cls.ancestors()
-                    if closest_ancestor in _.ancestors()
+                    anc
+                    for anc in cls.ancestors()
+                    if closest_ancestor in anc.ancestors()
                 )
         elif isinstance(generations, int):
             for entity in classes:
                 addancestors(entity, generations, ancestors)
         else:
-            ancestors.update(*(_.ancestors() for _ in classes))
+            ancestors.update(*(cls.ancestors() for cls in classes))
 
         if strict:
             return ancestors.difference(classes)
diff --git a/tests/ontopy_tests/test_graph.py b/tests/ontopy_tests/test_graph.py
index 7943e161d..0be76f9ec 100644
--- a/tests/ontopy_tests/test_graph.py
+++ b/tests/ontopy_tests/test_graph.py
@@ -76,8 +76,23 @@ class hasPartRenamed(owlready2.ObjectProperty):
     graph.add_legend()
     graph.save(tmpdir / "testonto.png")
 
+    with pytest.warns() as record:
+        graph2 = OntoGraph(
+            testonto,
+            testonto.TestClass,
+            relations="all",
+            addnodes=True,
+            edgelabels=None,
+        )
+        assert str(record[0].message) == (
+            "Style not defined for relation hasSpecialRelation. "
+            "Resorting to default style."
+        )
+    graph2.add_legend()
+    graph2.save(tmpdir / "testonto2.png")
+
 
-def test_emmo_graphs(emmo: "Ontology", repo_dir: "Path") -> None:
+def test_emmo_graphs(emmo: "Ontology", tmpdir: "Path") -> None:
     """Testing OntoGraph on various aspects of EMMO.
 
     Parameters:
@@ -90,7 +105,7 @@ def test_emmo_graphs(emmo: "Ontology", repo_dir: "Path") -> None:
     from ontopy.graph import OntoGraph, plot_modules
     from ontopy import get_ontology
     import warnings
-    tmpdir=repo_dir / "tmp"
+
     with warnings.catch_warnings():
         warnings.simplefilter("error")
         graph = OntoGraph(
@@ -218,7 +233,7 @@ def test_emmo_graphs(emmo: "Ontology", repo_dir: "Path") -> None:
         graph.add_entities(semiotic, relations="all", edgelabels=False)
         graph.add_legend()
         graph.save(tmpdir / "measurement.png", fmt="graphviz")
-        print('reductionistc')
+        print("reductionistc")
         # Reductionistic perspective
         graph = OntoGraph(
             emmo,
@@ -238,7 +253,6 @@ def test_emmo_graphs(emmo: "Ontology", repo_dir: "Path") -> None:
         graph.add_legend()
         graph.save(tmpdir / "Reductionistic.png")
 
-        print('add branch')
         # Reductionistic perspective, choose leaf_generations
         graph = OntoGraph(
             emmo,
@@ -248,7 +262,8 @@ def test_emmo_graphs(emmo: "Ontology", repo_dir: "Path") -> None:
             parents=2,
             edgelabels=None,
         )
-        graph.add_branch(emmo.Reductionistic,
+        graph.add_branch(
+            emmo.Reductionistic,
             leaves=[
                 emmo.Quantity,
                 emmo.String,
@@ -256,16 +271,37 @@ def test_emmo_graphs(emmo: "Ontology", repo_dir: "Path") -> None:
                 emmo.SymbolicConstruct,
                 emmo.Matter,
             ],
+        )
 
-            )
-            
         graph.add_legend()
         graph.save(tmpdir / "Reductionistic_addbranch.png")
 
+        graph2 = OntoGraph(
+            emmo,
+            emmo.Reductionistic,
+            relations="all",
+            addnodes=False,
+            # parents=2,
+            edgelabels=None,
+        )
+        graph2.add_branch(
+            emmo.Reductionistic,
+            leaves=[
+                emmo.Quantity,
+                emmo.String,
+                emmo.PrefixedUnit,
+                emmo.SymbolicConstruct,
+                emmo.Matter,
+            ],
+            include_parents=2,
+        )
+
+        graph2.add_legend()
+        graph2.save(tmpdir / "Reductionistic_addbranch_2.png")
+
         # View modules
 
         onto = get_ontology(
             "https://raw.githubusercontent.com/emmo-repo/EMMO/1.0.0-beta4/emmo.ttl"
         ).load()
         plot_modules(onto, tmpdir / "modules.png", ignore_redundant=True)
-