ENH add fontname argument in export_graphviz for non-English characters #18959

Zeroto521 · 2020-12-03T03:08:56Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Add an arg called fontname replace using the freezing Helvetica font.

Any other comments?

Well, this bug is very mad at me.
I have to handle it by changing the dot file font name and then using cmd dot -Tpdf tree -o dot_data.pdf again and again.

So just do it.

glemaitre

Before reviewing, could you revert all cosmetic changes linked to style issues? We intend to not add any of these changes and only focus on the code that need real changes because it makes it difficult to review otherwise.

glemaitre · 2020-12-16T08:58:10Z

.gitignore

@@ -79,3 +79,6 @@ _configtest.o.d
 sklearn/utils/_seq_dataset.pyx
 sklearn/utils/_seq_dataset.pxd
 sklearn/linear_model/_sag_fast.pyx
+
+# history codes
+.history


Could you revert this part

glemaitre · 2020-12-16T09:05:07Z

Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

Zeroto521 · 2020-12-16T09:29:04Z

Before reviewing, could you revert all cosmetic changes linked to style issues? We intend to not add any of these changes and only focus on the code that need real changes because it makes it difficult to review otherwise.

fine, I will do that later.

But when I open the _export.py, all lint errors come up to the editor.
And the screen is full of red or yellow lines.

glemaitre · 2020-12-16T09:34:21Z

But when I open the _export.py, all lint errors come up to the editor.
And the screen is full of red or yellow lines.

Yes this is possible. But we only check on the diff so you can ignore those.
This will get better when #18948 will be merged and that we all agree to use black.

Zeroto521 · 2020-12-17T01:27:26Z

I reset the branch and reopen pr this again

glemaitre

Since, fontname is created in _BaseTreeExporter, it could be shared with plot_tree (using matplotlib) and export_graphviz (using graphviz).

Could you update the plot_tree signature as well and pass the parameter to the underlying tree exported. It would also required to update the docstring of plot_tree and to change the tests.

glemaitre · 2020-12-17T09:01:08Z

doc/whats_new/v0.24.rst

@@ -785,6 +785,10 @@ Changelog
 - |Enhancement| :func:`tree.plot_tree` now uses colors from the matplotlib
  configuration settings. :pr:`17187` by `Andreas Müller`_.

+- |Fix| :func:`tree.export_graphviz`


Could you add the entry in v1.0.rst (you might need to merge master into your branch first). We already on our way to release 0.24 and we will probably be a bit too late to get this PR in.

The v1.0.rst file is not in my branch. The v1.0.rst was committed to master two days ago.

Yes you need to merge master into your branch to synchronize with master

glemaitre · 2020-12-17T09:09:25Z

doc/whats_new/v0.24.rst

@@ -785,6 +785,10 @@ Changelog
 - |Enhancement| :func:`tree.plot_tree` now uses colors from the matplotlib
  configuration settings. :pr:`17187` by `Andreas Müller`_.

+- |Fix| :func:`tree.export_graphviz`
+  Add fontname argument in plot_tree for non-English characters.


Suggested change

Add fontname argument in plot_tree for non-English characters.

Add `fontname` argument in :func:`tree.export_graphviz` for non-English characters.

Zeroto521 · 2020-12-17T10:00:51Z

Since, fontname is created in _BaseTreeExporter, it could be shared with plot_tree (using matplotlib) and export_graphviz (using graphviz).

Could you update the plot_tree signature as well and pass the parameter to the underlying tree exported. It would also required to update the docstring of plot_tree and to change the tests.

My test script as follows:

import graphviz

from sklearn import tree
from sklearn.datasets import load_iris

clf = tree.DecisionTreeClassifier()
iris = load_iris()

names = ['中文', 'にほんご', '조선말', 'ру́сский язы́к']

clf = clf.fit(iris.data, iris.target)
dot_data = tree.export_graphviz(
    clf, feature_names=names,
    filled=True, leaves_parallel=True, node_ids=True,
    rounded=True, special_characters=True, fontname='sans')
graph = graphviz.Source(dot_data)
graph.render("tree")

As for plot_tree function, I may need to find another example to test it.
By the way, I almost never use plot_tree.

Zeroto521 · 2020-12-18T02:28:38Z

Using matplotlib to plot the tree.
There also needs to add the following codes to make matplotlib know use what font to render.

A related issue, matplotlib/matplotlib#15062

plt.rcParams['font.family'] = [fontname]
plt.rcParams['font.sans-serif'] = [fontname]

glemaitre · 2020-12-18T09:30:30Z

doc/whats_new/v0.24.rst

@@ -792,6 +792,10 @@ Changelog
 - |Enhancement| :func:`tree.plot_tree` now uses colors from the matplotlib
  configuration settings. :pr:`17187` by `Andreas Müller`_.

+- |Fix| Add `fontname` argument in :func:`tree.export_graphviz` and


You can remove this entry now.

glemaitre · 2020-12-18T09:45:53Z

Regarding your test scrip in #18959 (comment), I think that we should include it and check the content as for the other test. It will allow us to make sure that we overwrite the fontname.

glemaitre · 2020-12-18T09:48:26Z

I think that for plot_tree, we can come with a minimal example such

from sklearn.datasets import load_iris
from sklearn import tree
clf = tree.DecisionTreeClassifier(random_state=0)
iris = load_iris()
clf = clf.fit(iris.data, iris.target)
tree_mpl = tree.plot_tree(clf)

Then, we can check

for annotation in tree_mpl:
    assert x.get_family() == ["fontname"]

I did not tested it but it should something around that line.

Zeroto521 · 2020-12-18T11:45:24Z

Regarding your test scrip in #18959 (comment), I think that we should include it and check the content as for the other test. It will allow us to make sure that we overwrite the fontname.

For this, there are four different kinds of languages. I couldn't find a font to render them all.
And the result is a picture or pdf. To check the result maybe not well.

glemaitre · 2020-12-18T11:52:59Z

For this, there are four different kinds of languages. I couldn't find a font to render them all.
And the result is a picture or pdf. To check the result maybe not well.

What I meant is to check the fontname=xxx in the content of the dot file to check that we overwrite the default helvetica.

Zeroto521 · 2020-12-23T02:15:41Z

I think that for plot_tree, we can come with a minimal example such

from sklearn.datasets import load_iris
from sklearn import tree
clf = tree.DecisionTreeClassifier(random_state=0)
iris = load_iris()
clf = clf.fit(iris.data, iris.target)
tree_mpl = tree.plot_tree(clf)

Then, we can check

for annotation in tree_mpl:
    assert x.get_family() == ["fontname"]

I did not tested it but it should something around that line.

When I tested it, I found matplotlib render font via plt.rcParams['font.family'] and plt.rcParams['font.sans-serif'].

So the fontname in tree.plot_tree no use. We should delete fontname for tree.plot_tree.

And my testing codes as follow:

import matplotlib.pyplot as plt
from sklearn import tree
from sklearn.datasets import load_iris

fontname = 'Microsoft YaHei'
plt.rcParams['font.family'] = [fontname]
plt.rcParams['font.sans-serif'] = [fontname]

iris = load_iris()
clf = tree.DecisionTreeClassifier(random_state=0)
clf = clf.fit(iris.data, iris.target)

names = ['中文', 'にほんご', '조선말', 'ру́сский язы́к']

plt.figure()
tree.plot_tree(
    clf, feature_names=names,
    filled=True, node_ids=True,
    rounded=True)
plt.show()

glemaitre · 2021-01-04T16:55:54Z

doc/whats_new/v1.0.rst

+...................
+
+- |Fix| Add `fontname` argument in :func:`tree.export_graphviz` and
+  :func:`tree.plot_tree` for non-English characters.


So we should remove plot_tree then?

glemaitre · 2021-01-04T16:57:16Z

sklearn/tree/_export.py

-              precision=3, ax=None, fontsize=None):
+              impurity=True, node_ids=False, proportion=False,
+              rotate='deprecated', rounded=False, precision=3,
+              ax=None, fontsize=None, fontname='helvetica'):


And we should revert the changes here as well for plot_tree

Zeroto521 · 2021-01-05T02:20:47Z

DeprecationWarning: 'wminkowski' metric is deprecated and will be removed in SciPy 1.8.0, use 'minkowski' instead.

This seems to need to update keywords to fix failing checks.

glemaitre

LGTM, thanks. We will need a second review.

glemaitre · 2021-01-05T09:34:16Z

@thomasjpfan @rth @jnothman Do you want to have a look since you looked at the former PR.

thomasjpfan

Thank you for the PR @Zeroto521 !

sklearn/tree/_export.py

thomasjpfan

This LGTM

@glemaitre The most recent commits makes it so that fontname changes the font regardness of rounded.

doc/whats_new/v1.0.rst

ogrisel

LGTM as well. Thanks!

github-actions bot added the module:tree label Dec 3, 2020

glemaitre reviewed Dec 16, 2020

View reviewed changes

glemaitre changed the title ~~Fix graphviz font rendering problem~~ FIX add fontname argument in plot_tree for non-English characters Dec 16, 2020

Zeroto521 closed this Dec 17, 2020

Zeroto521 added 2 commits December 17, 2020 10:35

FIX add fontname argument in plot_tree for non-English characters

448a3d5

Update v0.24.rst

2cd03cb

Zeroto521 reopened this Dec 17, 2020

Zeroto521 requested a review from glemaitre December 17, 2020 03:23

glemaitre reviewed Dec 17, 2020

View reviewed changes

Update v0.24.rst

bb6e603

Zeroto521 requested a review from glemaitre December 17, 2020 09:45

Zeroto521 added 3 commits December 17, 2020 18:14

Merge remote-tracking branch 'upstream/master' into graphviz-font

47550d7

Update v1.0.rst

f4741ec

FIX add fontname argument in plot_tree for non-English characters

8aaede3

Add changelog for #18959

01a2ef7

glemaitre reviewed Dec 18, 2020

View reviewed changes

Update v0.24.rst

f3fa121

Zeroto521 added 2 commits December 23, 2020 09:39

overwrite the default fontname to test the export_graphviz function

02ba737

Merge remote-tracking branch 'upstream/master' into graphviz-font

31021d0

Zeroto521 requested a review from glemaitre December 23, 2020 02:18

glemaitre reviewed Jan 4, 2021

View reviewed changes

Remove fontname for tree.plot_tree

384e63f

Zeroto521 changed the title ~~FIX add fontname argument in plot_tree for non-English characters~~ FIX add fontname argument in export_graphviz for non-English characters Jan 5, 2021

Zeroto521 requested a review from glemaitre January 5, 2021 02:21

glemaitre approved these changes Jan 5, 2021

View reviewed changes

thomasjpfan reviewed Jan 5, 2021

View reviewed changes

sklearn/tree/_export.py Outdated Show resolved Hide resolved

sklearn/tree/_export.py Show resolved Hide resolved

Zeroto521 added 3 commits January 6, 2021 09:11

Doc for tree.export_graphviz

74bd42a

Reduce fontname usage range, only in _DOTTreeExporter

b18c0fc

Fix fontname only used when rounded=True in tree.export_graphviz

3681b68

Zeroto521 requested a review from thomasjpfan January 6, 2021 02:10

Zeroto521 added 2 commits January 6, 2021 10:47

Update test_graphviz_toy for fontname

c115dcf

Update test_graphviz_toy for fontname

de4063f

thomasjpfan approved these changes Jan 6, 2021

View reviewed changes

thomasjpfan changed the title ~~FIX add fontname argument in export_graphviz for non-English characters~~ ENH add fontname argument in export_graphviz for non-English characters Jan 6, 2021

thomasjpfan reviewed Jan 6, 2021

View reviewed changes

doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved

ogrisel approved these changes Jan 10, 2021

View reviewed changes

ogrisel merged commit 34de1b9 into scikit-learn:master Jan 10, 2021

glemaitre mentioned this pull request Apr 22, 2021

Release 0.24.2 #19954

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH add fontname argument in export_graphviz for non-English characters #18959

ENH add fontname argument in export_graphviz for non-English characters #18959

	Add fontname argument in plot_tree for non-English characters.
	Add `fontname` argument in :func:`tree.export_graphviz` for non-English characters.

ENH add fontname argument in export_graphviz for non-English characters #18959

ENH add fontname argument in export_graphviz for non-English characters #18959

Conversation

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment