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.

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

Clarify documentation for GetFeatureWeights in TreeEnsembleModelParameters #3339

Merged
merged 1 commit into from Apr 15, 2019

Conversation

rogancarr
Copy link
Contributor

This PR clarifies the documentation for GetFeatureWeights() in TreeEnsembleModelParameters. Since IHaveFeatureWeights has become internal, the TreeEnsembleModelParameters are now the only place where this method is visible, and it is unclear from the API what this returns. This PR adds documentation to specify that the "weights" returned are the cumulative split gains for all the trees in the ensemble.

Fixes #1850

/// <param name="weights">a <see cref="VBuffer{T}"/> where feature weights would be assigned to.
/// The i-th element in <paramref name="weights"/> stores the weight of the i-th feature.</param>
/// <param name="weights">A <see cref="VBuffer{T}"/> to hold the cumulative split gain value for each feature.
/// The i-th element in <paramref name="weights"/> stores the cumulative split gain of the i-th feature.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @rogancarr! As discussed "weights" is a bit of an unfortunate word for "cumulative split gains," but it's probably too late to change the API at this point.

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.

Thank you @rogancarr !

@codecov
Copy link

codecov bot commented Apr 15, 2019

Codecov Report

Merging #3339 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3339      +/-   ##
==========================================
- Coverage   72.66%   72.66%   -0.01%     
==========================================
  Files         807      807              
  Lines      145191   145191              
  Branches    16223    16223              
==========================================
- Hits       105503   105502       -1     
  Misses      35268    35268              
- Partials     4420     4421       +1
Flag Coverage Δ
#Debug 72.66% <ø> (-0.01%) ⬇️
#production 68.19% <ø> (-0.01%) ⬇️
#test 88.97% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/Microsoft.ML.FastTree/FastTree.cs 80.71% <ø> (ø) ⬆️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs 86.1% <0%> (-0.16%) ⬇️

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@rogancarr rogancarr merged commit 0bb8163 into dotnet:master Apr 15, 2019
@rogancarr rogancarr deleted the 1850_tree_featuregain_docs branch April 15, 2019 21:59
rogancarr added a commit to rogancarr/machinelearning that referenced this pull request Apr 15, 2019
@dotnet dotnet locked as resolved and limited conversation to collaborators Mar 22, 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.

OLS FeatureWeights are not the model weights
3 participants