From 2cebcb315008ce2a0c8f1548e87b438623785add Mon Sep 17 00:00:00 2001
From: Benjamin Bossan <benjamin.bossan@gmail.com>
Date: Thu, 9 Mar 2023 12:57:48 +0100
Subject: [PATCH 1/4] Resolves #292, supersedes #296

As discussed in #296, instead of dealing with the issue what should
happen if the user sets model_card.model_diagram = False after
initializing with True, just remove the attribute from the Card class.
This way, there cannot be the false impression that the diagram can be
removed from the card by setting the attribute to False.

If users really need to remove the model diagram after enabling it at
first, they can just delete the corresponding section (or make it
invisible).

On top of that, a feature was added that passing a str to model_diagram
will use that str as the section name. Previously, always the default
section would be used (for skops template) or no section added (for
custom templates).
---
 skops/card/_model_card.py     | 47 +++++++++++++++++++++++++----------
 skops/card/tests/test_card.py | 33 ++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 13 deletions(-)

diff --git a/skops/card/_model_card.py b/skops/card/_model_card.py
index 15a8fe04..e1b1c41c 100644
--- a/skops/card/_model_card.py
+++ b/skops/card/_model_card.py
@@ -380,8 +380,15 @@ class Card:
         ``Path``/``str`` of the model or the actual model instance that will be
         documented. If a ``Path`` or ``str`` is provided, model will be loaded.
 
-    model_diagram: bool, default=True
-        Set to True if model diagram should be plotted in the card.
+    model_diagram: bool or str, default=True
+        If using the skops template, setting this to ``True`` will add the model
+        diagram, as generated by sckit-learn, to the default section. Passing a
+        string to ``model_diagram`` will instead use that string as the section
+        name for the diagram.
+
+        If using a non-skops template, the model diagram cannot be added
+        automatically unless a string is passed. It can, however, always be
+        added later using :meth:`Card.add_model_plot`.
 
     metadata: ModelCardData, optional
         :class:`huggingface_hub.ModelCardData` object. The contents of this
@@ -487,7 +494,6 @@ def __init__(
         trusted: bool = False,
     ) -> None:
         self.model = model
-        self.model_diagram = model_diagram
         self.metadata = metadata or ModelCardData()
         self.template = template
         self.trusted = trusted
@@ -495,13 +501,20 @@ def __init__(
         self._data: dict[str, Section] = {}
         self._metrics: dict[str, str | float | int] = {}
 
-        self._populate_template()
+        self._populate_template(model_diagram=model_diagram)
 
-    def _populate_template(self):
-        """If initialized with a template, use it to populate the card."""
-        if not self.template:
-            return
+    def _populate_template(self, model_diagram: bool | str):
+        """If initialized with a template, use it to populate the card.
+
+        Parameters
+        ----------
+        model_diagram: bool or str
+            If model_diagram is truthy and str, use str as section name. If
+            model_diagram is True, use default section for default template or
+            ignore for non-default template. If model_diagram is False, don't
+            add it.
 
+        """
         if isinstance(self.template, str) and (self.template not in VALID_TEMPLATES):
             valid_templates = ", ".join(f"'{val}'" for val in sorted(VALID_TEMPLATES))
             msg = (
@@ -510,15 +523,26 @@ def _populate_template(self):
             )
             raise ValueError(msg)
 
+        # default template
         if self.template == Templates.skops.value:
             self.add(**SKOPS_TEMPLATE)
             # for the skops template, automatically add some default sections
-            self.add_model_plot()
             self.add_hyperparams()
             self.add_get_started_code()
-        elif isinstance(self.template, Mapping):
+            if model_diagram:
+                if isinstance(model_diagram, str):
+                    self.add_model_plot(section=model_diagram)
+                else:
+                    self.add_model_plot()
+            return
+
+        # non-default template
+        if isinstance(self.template, Mapping):
             self.add(**self.template)
 
+        if model_diagram and isinstance(model_diagram, str):
+            self.add_model_plot(section=model_diagram)
+
     def get_model(self) -> Any:
         """Returns sklearn estimator object.
 
@@ -789,9 +813,6 @@ def add_model_plot(
         self : object
             Card object.
         """
-        if not self.model_diagram:
-            return self
-
         if section is None:
             if self.template == Templates.skops.value:
                 section = "Model description/Training Procedure/Model Plot"
diff --git a/skops/card/tests/test_card.py b/skops/card/tests/test_card.py
index 0a97a303..1058d115 100644
--- a/skops/card/tests/test_card.py
+++ b/skops/card/tests/test_card.py
@@ -177,6 +177,26 @@ def test_model_diagram_false(self):
         ).content
         assert result == "The model plot is below."
 
+    def test_model_diagram_str(self):
+        # if passing a str, use that as the section name
+        model = fit_model()
+        other_section_name = "Here is the model diagram"
+        model_card = Card(model, model_diagram=other_section_name)
+
+        # first check that default section only contains placeholder
+        result = model_card.select(
+            "Model description/Training Procedure/Model Plot"
+        ).format()
+        assert result == "The model plot is below."
+
+        # now check that the actual model diagram is in the other section
+        result = model_card.select(other_section_name).format()
+        assert result.startswith("The model plot is below.\n\n<style>#sk-")
+        assert "<style>" in result
+        assert result.endswith(
+            "<pre>LinearRegression()</pre></div></div></div></div></div>"
+        )
+
     def test_other_section(self, model_card):
         model_card.add_model_plot(section="Other section")
         result = model_card.select("Other section").content
@@ -204,6 +224,19 @@ def test_custom_template_no_section_raises(self, template):
         with pytest.raises(ValueError, match=msg):
             model_card.add_model_plot()
 
+    @pytest.mark.parametrize("template", CUSTOM_TEMPLATES)
+    def test_custom_template_init_str_works(self, template):
+        model = fit_model()
+        section_name = "Here is the model diagram"
+        model_card = Card(model, template=template, model_diagram=section_name)
+
+        result = model_card.select(section_name).format()
+        assert result.startswith("<style>#sk-")
+        assert "<style>" in result
+        assert result.endswith(
+            "<pre>LinearRegression()</pre></div></div></div></div></div>"
+        )
+
     def test_add_twice(self, model_card):
         # it's possible to add the section twice, even if it doesn't make a lot
         # of sense

From 239d9680f42185124bfbfdaecbd26c76212a6cc4 Mon Sep 17 00:00:00 2001
From: Benjamin Bossan <benjamin.bossan@gmail.com>
Date: Fri, 10 Mar 2023 12:29:37 +0100
Subject: [PATCH 2/4] Fix missing type annotation

---
 skops/card/_model_card.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/skops/card/_model_card.py b/skops/card/_model_card.py
index e1b1c41c..e8339b5b 100644
--- a/skops/card/_model_card.py
+++ b/skops/card/_model_card.py
@@ -488,7 +488,7 @@ class Card:
     def __init__(
         self,
         model,
-        model_diagram: bool = True,
+        model_diagram: bool | str = True,
         metadata: ModelCardData | None = None,
         template: Literal["skops"] | dict[str, str] | None = "skops",
         trusted: bool = False,

From 1c160b39e3bb09a8a730f30899f4b5cf583be378 Mon Sep 17 00:00:00 2001
From: Benjamin Bossan <benjamin.bossan@gmail.com>
Date: Fri, 10 Mar 2023 15:21:28 +0100
Subject: [PATCH 3/4] model_diagram can be set to "auto"

"auto" is the new default and it will add a model_diagram only if the
default template is used, preserving the current behavior.
---
 skops/card/_model_card.py     | 52 ++++++++++++++++++++---------------
 skops/card/tests/test_card.py | 28 +++++++++++++++++++
 2 files changed, 58 insertions(+), 22 deletions(-)

diff --git a/skops/card/_model_card.py b/skops/card/_model_card.py
index e8339b5b..d52efe54 100644
--- a/skops/card/_model_card.py
+++ b/skops/card/_model_card.py
@@ -380,15 +380,17 @@ class Card:
         ``Path``/``str`` of the model or the actual model instance that will be
         documented. If a ``Path`` or ``str`` is provided, model will be loaded.
 
-    model_diagram: bool or str, default=True
-        If using the skops template, setting this to ``True`` will add the model
-        diagram, as generated by sckit-learn, to the default section. Passing a
-        string to ``model_diagram`` will instead use that string as the section
-        name for the diagram.
-
-        If using a non-skops template, the model diagram cannot be added
-        automatically unless a string is passed. It can, however, always be
-        added later using :meth:`Card.add_model_plot`.
+    model_diagram: bool or "auto" or str, default="auto"
+        If using the skops template, setting this to ``True`` or ``"auto"`` will
+        add the model diagram, as generated by sckit-learn, to the default
+        section. Passing a string to ``model_diagram`` will instead use that
+        string as the section name for the diagram. Set to ``False`` to not
+        include the model diagram.
+
+        If using a non-skops template, passing ``"auto"`` won't add the model
+        diagram because there is no pre-defined section to put it. The model
+        diagram can, however, always be added later using
+        :meth:`Card.add_model_plot`.
 
     metadata: ModelCardData, optional
         :class:`huggingface_hub.ModelCardData` object. The contents of this
@@ -488,7 +490,7 @@ class Card:
     def __init__(
         self,
         model,
-        model_diagram: bool | str = True,
+        model_diagram: bool | Literal["auto"] | str = "auto",
         metadata: ModelCardData | None = None,
         template: Literal["skops"] | dict[str, str] | None = "skops",
         trusted: bool = False,
@@ -503,16 +505,19 @@ def __init__(
 
         self._populate_template(model_diagram=model_diagram)
 
-    def _populate_template(self, model_diagram: bool | str):
+    def _populate_template(self, model_diagram: bool | Literal["auto"] | str):
         """If initialized with a template, use it to populate the card.
 
         Parameters
         ----------
-        model_diagram: bool or str
-            If model_diagram is truthy and str, use str as section name. If
-            model_diagram is True, use default section for default template or
-            ignore for non-default template. If model_diagram is False, don't
-            add it.
+        model_diagram: bool or "auto" or str
+            If using the default template, ``"auto"`` and ``True`` will add the
+            diagram in its default section. If using a custom template,
+            ``"auto"`` will not add the diagram, and passing ``True`` will
+            result in an error. For either, passing ``False`` will result in the
+            model diagram being omitted, and passing a string (other than
+            ``"auto"``) will put the model diagram into a section corresponding
+            to that string.
 
         """
         if isinstance(self.template, str) and (self.template not in VALID_TEMPLATES):
@@ -529,19 +534,22 @@ def _populate_template(self, model_diagram: bool | str):
             # for the skops template, automatically add some default sections
             self.add_hyperparams()
             self.add_get_started_code()
-            if model_diagram:
-                if isinstance(model_diagram, str):
-                    self.add_model_plot(section=model_diagram)
-                else:
-                    self.add_model_plot()
+
+            if (model_diagram is True) or (model_diagram == "auto"):
+                self.add_model_plot()
+            elif isinstance(model_diagram, str):
+                self.add_model_plot(section=model_diagram)
             return
 
         # non-default template
         if isinstance(self.template, Mapping):
             self.add(**self.template)
 
-        if model_diagram and isinstance(model_diagram, str):
+        if isinstance(model_diagram, str) and (model_diagram != "auto"):
             self.add_model_plot(section=model_diagram)
+        elif model_diagram is True:
+            # will trigger an error
+            self.add_model_plot()
 
     def get_model(self) -> Any:
         """Returns sklearn estimator object.
diff --git a/skops/card/tests/test_card.py b/skops/card/tests/test_card.py
index 1058d115..f9d8e0d8 100644
--- a/skops/card/tests/test_card.py
+++ b/skops/card/tests/test_card.py
@@ -237,6 +237,34 @@ def test_custom_template_init_str_works(self, template):
             "<pre>LinearRegression()</pre></div></div></div></div></div>"
         )
 
+    def test_default_template_and_model_diagram_true(self, model_card):
+        # setting model_diagram=True should not change anything vs auto with the
+        # default template
+        model = fit_model()
+        model_card = Card(model, model_diagram=True)
+        result = model_card.select(
+            "Model description/Training Procedure/Model Plot"
+        ).content
+        # don't compare whole text, as it's quite long and non-deterministic
+        assert result.startswith("The model plot is below.\n\n<style>#sk-")
+        assert "<style>" in result
+        assert result.endswith(
+            "<pre>LinearRegression()</pre></div></div></div></div></div>"
+        )
+
+    @pytest.mark.parametrize("template", CUSTOM_TEMPLATES)
+    def test_custom_template_and_model_diagram_true(self, model_card, template):
+        # in contrast to the previous test, when setting model_diagram=True but
+        # using a custom template, we expect an error during initialization of
+        # the model cord
+        model = fit_model()
+        msg = (
+            "You are trying to add a model plot but you're using a custom template, "
+            "please pass the 'section' argument to determine where to put the content"
+        )
+        with pytest.raises(ValueError, match=msg):
+            Card(model, template=template, model_diagram=True)
+
     def test_add_twice(self, model_card):
         # it's possible to add the section twice, even if it doesn't make a lot
         # of sense

From b445d74e54518b63e6ae7b35787b282ee069e8d9 Mon Sep 17 00:00:00 2001
From: Benjamin Bossan <benjamin.bossan@gmail.com>
Date: Mon, 13 Mar 2023 11:33:39 +0100
Subject: [PATCH 4/4] Reviewer comment: Expl. name section for diagram

---
 skops/card/_model_card.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/skops/card/_model_card.py b/skops/card/_model_card.py
index d52efe54..5d9942f5 100644
--- a/skops/card/_model_card.py
+++ b/skops/card/_model_card.py
@@ -383,9 +383,10 @@ class Card:
     model_diagram: bool or "auto" or str, default="auto"
         If using the skops template, setting this to ``True`` or ``"auto"`` will
         add the model diagram, as generated by sckit-learn, to the default
-        section. Passing a string to ``model_diagram`` will instead use that
-        string as the section name for the diagram. Set to ``False`` to not
-        include the model diagram.
+        section, i.e "Model description/Training Procedure/Model Plot". Passing
+        a string to ``model_diagram`` will instead use that string as the
+        section name for the diagram. Set to ``False`` to not include the model
+        diagram.
 
         If using a non-skops template, passing ``"auto"`` won't add the model
         diagram because there is no pre-defined section to put it. The model