Skip to content

FIX escape double quotes when exporting tree with Graphviz#17575

Merged
adrinjalali merged 9 commits into
scikit-learn:mainfrom
smola:graphviz-escape
Oct 28, 2024
Merged

FIX escape double quotes when exporting tree with Graphviz#17575
adrinjalali merged 9 commits into
scikit-learn:mainfrom
smola:graphviz-escape

Conversation

@smola

@smola smola commented Jun 12, 2020

Copy link
Copy Markdown
Contributor

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Double quotes in graphviz labels need escaping. Escape class and feature
names to avoid breaking the result.

Any other comments?

@smola smola force-pushed the graphviz-escape branch 2 times, most recently from 20ebf08 to 7677fe3 Compare June 12, 2020 11:23

@ogrisel ogrisel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some comments / questions:

Comment thread sklearn/tree/_export.py Outdated
Comment thread sklearn/tree/tests/test_export.py Outdated
'2 [label="gini = 0.0\\nsamples = 3\\nvalue = [0, 3]"] ;\n' \
'0 -> 2 [labeldistance=2.5, labelangle=-45, ' \
'headlabel="False"] ;\n' \
'}'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To improve readability may I suggest the following idiom:

from textwrap import dedent

...
    contents1 = export_graphviz(clf,
                                feature_names=["feature\"0\"", "feature\"1\""],
                                out_file=None)
    contents2 = dedent("""\
        digraph Tree {
        node [shape=box] ;
        ...
        }""")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was hesitant to use a convention different from the surrounding code, since all test cases in the file use this form. Do you think it's worth changing only for this new case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ogrisel So would you prefer the code using the same convention as the surrounding test cases? Or the one you suggested?

Double quotes in graphviz labels need escaping. Escape class and feature
names to avoid breaking the result.
@smola smola force-pushed the graphviz-escape branch from 7677fe3 to 577843e Compare June 13, 2020 10:02
@smola smola requested a review from ogrisel August 31, 2020 15:16
Base automatically changed from master to main January 22, 2021 10:52
@glemaitre glemaitre self-requested a review March 12, 2024 14:42
@github-actions

github-actions Bot commented Mar 12, 2024

Copy link
Copy Markdown

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: fd70673. Link to the linter CI: here

@glemaitre glemaitre added this to the 1.6 milestone May 21, 2024
@glemaitre glemaitre changed the title escape strings for export_graphviz FIX escape double quotes when exporting tree with Graphviz May 21, 2024

@glemaitre glemaitre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this fix make sense. I added an entry in what's new and solve the failure in tests due to changes that have been merged in between regarding the font style.

@glemaitre glemaitre added the Bug label May 21, 2024
@adrinjalali adrinjalali enabled auto-merge (squash) October 22, 2024 14:29
@adrinjalali adrinjalali merged commit 9238fe0 into scikit-learn:main Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants