Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature edge_info=none #147

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+12 −13
Open

Feature edge_info=none #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f4c870e
Update _event.py
JackStorrorCarter Nov 25, 2024
bb62f9d
Update _ceg.py
JackStorrorCarter Nov 25, 2024
eb531db
Update _event.py
JackStorrorCarter Nov 25, 2024
c6a9bcf
Update _staged.py
JackStorrorCarter Nov 25, 2024
2f439b4
Update _event.py
JackStorrorCarter Nov 25, 2024
7ace5af
Update _ceg.py
JackStorrorCarter Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/cegpy/graphs/_ceg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,9 @@ def _merge_nodes(self, nodes_to_merge: List[Tuple[str]]):

def dot_graph(self, edge_info: str = "probability") -> pdp.Dot:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def dot_graph(self, edge_info: str = "probability") -> pdp.Dot:
def dot_graph(self, edge_info: Literal["count", "prior", "posterior", "probability"] | None = "probability") -> pdp.Dot:

Literal["count", "prior", "posterior", "probability"] -- this says that only "count", "prior", "posterior" or "probability" are acceptable input parameters.

| -- this means "or"

None -- None is its own type in Python, and so we shouldn't be using "none" as a string.

With the above setting, the user will need to explicitly pass None for no information to be displayed.
Alternatively, we can make None the default, so the user needs to explicitly pass one of the acceptable string entries for any information to be displayed.

What do you think?

"""Returns Dot graph representation of the CEG.
:param edge_info: Optional - Chooses which summary measure to be displayed
on edges. Defaults to "count".
Options: ["count", "prior", "posterior", "probability"]
:param edge_info: Optional - Chooses which summary measure to be displayed on edges. Defaults to "count". Options: ["count", "prior", "posterior", "probability", "none"]
:type edge_info: str
:return: A graphviz Dot representation of the graph.
:rtype: pydotplus.Dot"""
return self._generate_dot_graph(edge_info=edge_info)
Expand All @@ -209,6 +207,8 @@ def _generate_dot_graph(self, edge_info="probability"):
for (src, dst, label), attribute in edge_info_dict.items():
if edge_info == "count":
edge_details = str(label) + "\n" + str(attribute)
elif edge_info == "none":
edge_details = str(label)
else:
edge_details = f"{label}\n{float(attribute):.2f}"
Comment on lines 207 to 213
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (src, dst, label), attribute in edge_info_dict.items():
if edge_info == "count":
edge_details = str(label) + "\n" + str(attribute)
elif edge_info == "none":
edge_details = str(label)
else:
edge_details = f"{label}\n{float(attribute):.2f}"
for (src, dst, label), attribute in edge_info_dict.items():
if not edge_info:
edge_details = str(label)
elif edge_info == "count":
edge_details = str(label) + "\n" + str(attribute)
else:
edge_details = f"{label}\n{float(attribute):.2f}"

These are the changes we'd need to make if using None. The truth value of a None type is always False and for a non-None type will be True. So checking it as if not edge_info will only be True when edge_info is None. Let me know if doesn't make sense and I'll give a better example

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to do this:

        edge_details = str(label)        
        for (src, dst, label), attribute in edge_info_dict.items():

            if edge_info == "count":
                edge_details = edge_details + "\n" + str(attribute)
            else:
                edge_details = f"{edge_details}\n{float(attribute):.2f}"

Since edge_details always include str(label), you can add initialise the edge_details variable with the label and then don't need to explicitly check for the None case.


Expand Down Expand Up @@ -258,8 +258,7 @@ def create_figure(
:type filename: str
:param edge_info: Optional - Chooses which summary measure to be displayed on
edges. Value can take: "count", "prior", "posterior", "probability"
:param edge_info: Optional - Chooses which summary measure to be displayed on edges. Defaults to "count". Options: ["count", "prior", "posterior", "probability", "none"]
:type edge_info: str
:return: The event tree Image object.
Expand Down
9 changes: 5 additions & 4 deletions src/cegpy/trees/_event.py
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as in src/cegpy/graphs/_ceg.py

Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,8 @@ def categories_per_variable(self) -> Dict:

def dot_graph(self, edge_info: str = "count") -> pdp.Dot:
"""Returns Dot graph representation of the event tree.
:param edge_info: Optional - Chooses which summary measure to be displayed on edges.
In event trees, only "count" can be displayed, so this can be omitted.
:param edge_info: Optional - Chooses which summary measure to be displayed on edges. Defaults to "count". Options: ["count", "none"]
:type edge_info: str
:type edge_info: str
:return: A graphviz Dot representation of the graph.
Expand All @@ -256,6 +256,8 @@ def _generate_dot_graph(self, fill_colour=None, edge_info="count"):
for edge, attribute in edge_info_dict.items():
if edge_info == "count":
edge_details = str(edge[2]) + "\n" + str(attribute)
elif edge_info == "none":
edge_details = str(edge[2])
else:
edge_details = f"{edge[2]}\n{float(attribute):.2f}"

Expand Down Expand Up @@ -321,8 +323,7 @@ def create_figure(
:type filename: str
:param edge_info: Optional - Chooses which summary measure to be displayed on edges.
In event trees, only "count" can be displayed, so this can be omitted.
:param edge_info: Optional - Chooses which summary measure to be displayed on edges. Defaults to "count". Options: ["count", "none"]
:type edge_info: str
:return: The event tree Image object.
Expand Down
5 changes: 2 additions & 3 deletions src/cegpy/trees/_staged.py
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as in src/cegpy/graphs/_ceg.py

Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,7 @@ def calculate_AHC_transitions(
def dot_graph(self, edge_info: str = "count", staged: bool = True):
"""Returns Dot graph representation of the staged tree.
:param edge_info: Optional - Chooses which summary measure to be displayed on edges. Defaults to "count". Options: ["count", "prior", "posterior", "probability"]
:param edge_info: Optional - Chooses which summary measure to be displayed on edges. Defaults to "count". Options: ["count", "prior", "posterior", "probability", "none"]
:type edge_info: str
:param staged: if True, returns the coloured staged tree,
Expand Down Expand Up @@ -817,8 +817,7 @@ def create_figure(
:type filename: str
:param edge_info: Optional - Chooses which summary measure to be displayed on edges.
In event trees, only "count" can be displayed, so this can be omitted.
:param edge_info: Optional - Chooses which summary measure to be displayed on edges. Defaults to "count". Options: ["count", "prior", "posterior", "probability", "none"]
:type edge_info: str
:param staged: if True, returns the coloured staged tree,
Expand Down