Skip to content

Address minor comments in #2243 #2369

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.

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

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Conversation

wschin
Copy link
Member

@wschin wschin commented Feb 1, 2019

This paper continues addressing some small comments left in #2243.

@wschin wschin self-assigned this Feb 1, 2019
@wschin wschin requested review from Ivanidzo4ka and sfilipi February 1, 2019 17:32
@shauheen
Copy link
Contributor

shauheen commented Feb 1, 2019

As title.

Thanks @wschin , even though this is a small PR, please edit the description and at least mention the other PR here so that they are actually linked.

@wschin
Copy link
Member Author

wschin commented Feb 1, 2019

@shauheen, they are linked. If you go to #2243, there is a footprint. In other words, # followed by a number in commit messages are correctly parsed by github.

@codecov
Copy link

codecov bot commented Feb 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@3eccc93). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #2369   +/-   ##
=========================================
  Coverage          ?   71.24%           
=========================================
  Files             ?      783           
  Lines             ?   140721           
  Branches          ?    16086           
=========================================
  Hits              ?   100261           
  Misses            ?    36006           
  Partials          ?     4454
Flag Coverage Δ
#Debug 71.24% <100%> (?)
#production 67.6% <100%> (?)
#test 85.28% <ø> (?)

/// first leaf in the underlying <see cref="_tree"/>.
/// <see langword="~"/>(-1)-th leaf in the underlying <see cref="_tree"/>. Note that <see langword="~"/> is the
/// bitwise complement operator in C#; for details, see
/// https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/bitwise-complement-operator.
Copy link
Member

Choose a reason for hiding this comment

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

see the documentation about the bitwise complement operator

Copy link
Member

@sfilipi sfilipi left a comment

Choose a reason for hiding this comment

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

:shipit:

@Ivanidzo4ka
Copy link
Contributor

    public IReadOnlyList<double> TreeWeights { get; }

I think I asked this question in previous PR, but I can't find that any more, so I ask again.
I don't see them been used during prediction, why we expose them?


Refers to: src/Microsoft.ML.FastTree/TreeEnsemble.cs:25 in 2eb1ef2. [](commit_id = 2eb1ef2, deletion_comment = False)

@wschin
Copy link
Member Author

wschin commented Feb 2, 2019

    public IReadOnlyList<double> TreeWeights { get; }

I think I asked this question in previous PR, but I can't find that any more, so I ask again.
I don't see them been used during prediction, why we expose them?

Refers to: src/Microsoft.ML.FastTree/TreeEnsemble.cs:25 in 2eb1ef2. [](commit_id = 2eb1ef2, deletion_comment = False)

@TomFinley, do you have any idea about exposing trees' weights to users? They are extracted from InternalRegressionTree and stored in (new) TreeEnsemble.

@TomFinley
Copy link
Contributor

@TomFinley, do you have any idea about exposing trees' weights to users? They are extracted from InternalRegressionTree and stored in (new) TreeEnsemble.

Trees definitely do have weights attached to them. If some code was written that does not account for them, then we ought to address that. Now, functionally I expect this won't have an effect since the weights are under most situations 1, but, that's besides the point.

Here's an example where the weights seem to be taken into account. You can find other usages here and there.

I think having weights on the trees themselves is useful, and we should continue to have them.

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Thanks @wschin !

@TomFinley TomFinley merged commit 56ae4eb into dotnet:master Feb 7, 2019
@wschin wschin deleted the small-tree-change branch February 7, 2019 23:11
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants