DOC Fix tree explanation of `tree_.value` in example #29331

adam2392 · 2024-06-21T14:48:20Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Fixes the explanation of tree_.value as suggested in #28921 (comment).

A follow-on PR that changes the name of tree_.value with a deprecation to tree_.weighted_proportion seems better and more explicit.

Any other comments?

cc: @lesteve

Signed-off-by: Adam Li <adam2392@gmail.com>

github-actions · 2024-06-21T14:49:36Z

✔️ Linting Passed

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

_{Generated for commit: dfcd9e5. Link to the linter CI: here}

ArturoAmorQ

Hi @adam2392, thanks for addressing this issue :) Here's a couple of comments:

On top of the API changelog entry, we might want to also include a versionchanged directive for each of the attributes on those estimators.
For the FIX changelog entry, I think you are linking to an unrelated PR, but in any case, the proportion parameter of plot_tree function has not changed in version 1.6 as far as I know, or am I misunderstanding something?

adam2392 · 2024-06-24T16:55:00Z

On top of the API changelog entry, we might want to also include a versionchanged directive for each of the attributes on those estimators.

The tree_.value is currently an undocumented attributed, so should I add a versionchanged to the tree_ attribute in each of these tree classes?

For the FIX changelog entry, I think you are linking to an unrelated PR, but in any case, the proportion parameter of plot_tree function has not changed in version 1.6 as far as I know, or am I misunderstanding something?

The PR that I linked is when the behavior changed such that tree_.value in Cython stores proportions instead of absolute counts. The plot_tree does not have any API changes you're right, so I suppose I can change the fix changelog entry for v1.6 and remove that altogether, since this is just a patch at the documentation/example level.

Signed-off-by: Adam Li <adam2392@gmail.com>

adam2392 · 2024-06-25T12:17:31Z

The issues raised are fixed now:

only show floating point up to 3 decimal places when printing tree_.value in the example
show proportions when plotting tree (here I added an API change to set proportion=True by default, since this is in line with the behavior of tree_.value. (pending commit)

ref: https://output.circle-artifacts.com/output/job/bdc192fe-d4f9-4649-9ff0-03ddfb038abf/artifacts/0/doc/auto_examples/tree/plot_unveil_tree_structure.html

Signed-off-by: Adam Li <adam2392@gmail.com>

ArturoAmorQ · 2024-06-25T14:17:37Z

I would rather not change the behavior of plot_tree for backward compatibility and just set proportion=True in the example. Could you please revert the latest commit?

Signed-off-by: Adam Li <adam2392@gmail.com>

adam2392 · 2024-06-25T14:19:06Z

Kay that makes sense! Done

ArturoAmorQ

A couple of final comments but otherwise LGTM :)

sklearn/tree/_export.py

examples/tree/plot_unveil_tree_structure.py

ArturoAmorQ · 2024-06-26T09:12:38Z

doc/whats_new/v1.4.rst

+  :pr:`13649` by :user:`Samuel Ronsin <samronsin>`, initiated by
+  :user:`Patrick O'Reilly <pat-oreilly>`.


Pinging @glemaitre for his opinion to rather linking #27639 here.

#27639 indeed seems more appropriate, since this is where the choice of switching to a weighted proportion was made IIUC.

For better visibility I would also put it in a changed models section i.e. a lot more towards the top. For example there is such a section in 1.5 but a new one needs to be added for 1.4.

Edited and moved to the top.

lesteve

Thanks a lot, a few comments from me

lesteve · 2024-06-27T09:41:42Z

doc/whats_new/v1.4.rst

+  :pr:`13649` by :user:`Samuel Ronsin <samronsin>`, initiated by
+  :user:`Patrick O'Reilly <pat-oreilly>`.


#27639 indeed seems more appropriate, since this is where the choice of switching to a weighted proportion was made IIUC.

For better visibility I would also put it in a changed models section i.e. a lot more towards the top. For example there is such a section in 1.5 but a new one needs to be added for 1.4.

sklearn/tree/_classes.py

examples/tree/plot_unveil_tree_structure.py

Signed-off-by: Adam Li <adam2392@gmail.com>

adam2392 · 2024-06-27T13:57:32Z

Thanks for the review @lesteve! I have addressed your comments in 7343096

Signed-off-by: Adam Li <adam2392@gmail.com>

lesteve · 2024-07-01T15:30:42Z

examples/tree/plot_unveil_tree_structure.py

+#
+# One could convert this to the absolute weighted number of samples reaching a node,
+# by multiplying this number by `tree_.weighted_n_node_samples[node_idx]` for the
+# given node, or by `tree_.n_node_samples[node_idx]` for the unweighted number of


I am a bit unsure about multiplying tree_.value (proportion of samples taking into account sample weights) by tree_.n_node_samples[node_idx] which does not take sample weights into account. It seems to me that the output of this multiplication has not a clear meaning, but maybe I am missing something ...

Maybe only mention tree_.weighted_n_nodes_sample and mention that sample weights are not used in this example, which is the same as being one for all the samples?

Done: 3c1f24a

lesteve · 2024-07-01T15:35:10Z

examples/tree/plot_unveil_tree_structure.py


-tree.plot_tree(clf)
+tree.plot_tree(clf, proportion=True)


I guess that works for this example, but in an ideal world (not this PR) it would be nice if by default plot_tree would show something that matches tree_.value. Is it worth doing a 2 release deprecation period where you say that proportion default value is going to switch from False to True. Could we bend our rules and switch to proportion=True directly?

I'd slightly lean towards bending our rules and switch to proportion=True by default but this is a bit controversial.

I originally did deprecation, but @ArturoAmorQ (#29331 (comment)) suggested I keep the default behavior.

I honestly think either way is okay as this is like power-user stuff

Signed-off-by: Adam Li <adam2392@gmail.com>

lesteve · 2024-07-02T03:57:27Z

OK thanks a lot for your patience and continued work on this @adam2392, let's merge this one!

lesteve · 2024-07-02T12:30:42Z

@adam2392 if you feel adventurous you may want to open an issue about aggressively switching the default to proportion=True in plot_tree in order to match tree.value and get other maintainers feeling about it. Not sure if other visualization functions are affected by this to be 100% honest ...

) Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com> Co-authored-by: Loïc Estève <loic.esteve@ymail.com>

Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com> Co-authored-by: Loïc Estève <loic.esteve@ymail.com>

Fix tree explanation

3102d31

Signed-off-by: Adam Li <adam2392@gmail.com>

github-actions bot added the Documentation label Jun 21, 2024

ArturoAmorQ reviewed Jun 24, 2024

View reviewed changes

Address arturo changes

b653171

Signed-off-by: Adam Li <adam2392@gmail.com>

adam2392 requested a review from ArturoAmorQ June 24, 2024 17:09

Merge branch 'main' into treedescription

1f5640f

Change default parameter

34b20a2

Signed-off-by: Adam Li <adam2392@gmail.com>

Revert

db6b77d

Signed-off-by: Adam Li <adam2392@gmail.com>

ArturoAmorQ approved these changes Jun 26, 2024

View reviewed changes

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

examples/tree/plot_unveil_tree_structure.py Outdated Show resolved Hide resolved

Apply suggestions from code review

ab30bb4

ArturoAmorQ reviewed Jun 26, 2024

View reviewed changes

adam2392 added 2 commits June 26, 2024 08:03

Merge branch 'main' into treedescription

0b0450a

Merge branch 'main' into treedescription

c6de572

ArturoAmorQ changed the title ~~API, FIX, DOC Fix tree explanation of tree_.value in example~~ DOC Fix tree explanation of tree_.value in example Jun 27, 2024

lesteve reviewed Jun 27, 2024

View reviewed changes

Update the comments from lesteve

7343096

Signed-off-by: Adam Li <adam2392@gmail.com>

Remove extra changes

e50b2c4

Signed-off-by: Adam Li <adam2392@gmail.com>

adam2392 requested a review from lesteve June 27, 2024 13:58

Update v1.4.rst

267d1ea

lesteve reviewed Jul 1, 2024

View reviewed changes

adam2392 added 2 commits July 1, 2024 11:36

Address lesteve comment

3c1f24a

Signed-off-by: Adam Li <adam2392@gmail.com>

Merge branch 'main' into treedescription

dfcd9e5

adam2392 requested a review from lesteve July 1, 2024 15:37

lesteve merged commit 610d4f7 into scikit-learn:main Jul 2, 2024
30 checks passed

adam2392 deleted the treedescription branch July 2, 2024 12:11

adam2392 mentioned this pull request Jul 2, 2024

API Change default argument for proportion to be True in tree.plot_tree #29383

Open

glemaitre pushed a commit that referenced this pull request Sep 11, 2024

DOC Fix tree explanation of tree_.value in example (#29331)

511fb4d

Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com> Co-authored-by: Loïc Estève <loic.esteve@ymail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC Fix tree explanation of `tree_.value` in example #29331

DOC Fix tree explanation of `tree_.value` in example #29331

adam2392 commented Jun 21, 2024

github-actions bot commented Jun 21, 2024 •

edited

Loading

ArturoAmorQ left a comment

adam2392 commented Jun 24, 2024 •

edited

Loading

adam2392 commented Jun 25, 2024

ArturoAmorQ commented Jun 25, 2024

adam2392 commented Jun 25, 2024 •

edited

Loading

ArturoAmorQ left a comment

ArturoAmorQ Jun 26, 2024

lesteve Jun 27, 2024 •

edited

Loading

adam2392 Jun 27, 2024

lesteve left a comment

lesteve Jun 27, 2024 •

edited

Loading

adam2392 commented Jun 27, 2024

lesteve Jul 1, 2024

adam2392 Jul 1, 2024

lesteve Jul 1, 2024

adam2392 Jul 1, 2024

adam2392 Jul 1, 2024

lesteve commented Jul 2, 2024

lesteve commented Jul 2, 2024

		:pr:`13649` by :user:`Samuel Ronsin <samronsin>`, initiated by
		:user:`Patrick O'Reilly <pat-oreilly>`.


		tree.plot_tree(clf)
		tree.plot_tree(clf, proportion=True)

DOC Fix tree explanation of tree_.value in example #29331

DOC Fix tree explanation of tree_.value in example #29331

Conversation

adam2392 commented Jun 21, 2024

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

github-actions bot commented Jun 21, 2024 • edited Loading

✔️ Linting Passed

ArturoAmorQ left a comment

Choose a reason for hiding this comment

adam2392 commented Jun 24, 2024 • edited Loading

adam2392 commented Jun 25, 2024

ArturoAmorQ commented Jun 25, 2024

adam2392 commented Jun 25, 2024 • edited Loading

ArturoAmorQ left a comment

Choose a reason for hiding this comment

ArturoAmorQ Jun 26, 2024

Choose a reason for hiding this comment

lesteve Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

adam2392 Jun 27, 2024

Choose a reason for hiding this comment

lesteve left a comment

Choose a reason for hiding this comment

lesteve Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

adam2392 commented Jun 27, 2024

lesteve Jul 1, 2024

Choose a reason for hiding this comment

adam2392 Jul 1, 2024

Choose a reason for hiding this comment

lesteve Jul 1, 2024

Choose a reason for hiding this comment

adam2392 Jul 1, 2024

Choose a reason for hiding this comment

adam2392 Jul 1, 2024

Choose a reason for hiding this comment

lesteve commented Jul 2, 2024

lesteve commented Jul 2, 2024

DOC Fix tree explanation of `tree_.value` in example #29331

DOC Fix tree explanation of `tree_.value` in example #29331

github-actions bot commented Jun 21, 2024 •

edited

Loading

adam2392 commented Jun 24, 2024 •

edited

Loading

adam2392 commented Jun 25, 2024 •

edited

Loading

lesteve Jun 27, 2024 •

edited

Loading

lesteve Jun 27, 2024 •

edited

Loading