-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Update the parameters description in the summary as described in the issue 2177 #2432
Update the parameters description in the summary as described in the issue 2177 #2432
Conversation
to be honest I don't know why the code coverage failed... Is there an issue with master? |
We have sporadic failures due to the random nature of our tests, I just restarted the code coverage bot. Thanks for your PR! |
Codecov Report
@@ Coverage Diff @@
## master #2432 +/- ##
==========================================
+ Coverage 71.5% 71.5% +<.01%
==========================================
Files 801 801
Lines 142036 142033 -3
Branches 16151 16148 -3
==========================================
+ Hits 101559 101560 +1
+ Misses 36004 36002 -2
+ Partials 4473 4471 -2
|
@sfilipi do you mean in issue to unify all column or only specific one? |
@HerraHak thanks for your PR. I have edited the original issue to describe other feedback that we have had. this is how those parameters should look like: /// The name of the label column. There are many of them, through the code-base. If you don't intend to fix all of them, please don;t close the issue. @Ivanidzo4ka does the above look good? as long as we standardize to something. |
/// <param name="weights">The optional weights column.</param> | ||
/// <param name="labelColumn">The name of the label column.</param> | ||
/// <param name="featureColumn">The name of the feature column.</param> | ||
/// <param name="weights">The name of the optional weights column.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so weightsColumnName, labelColumnName, featureColumnName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change the name of the parameters too or is a different issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more than happy with proposed language. I just wonder what is criteria to close issue :) In reply to: 461206771 [](ancestors = 461206771) |
So if I understand correctly, the language like it is in the pull request is what we want?
If that's the case, I can look for other occurrences in the code and change that.
|
Huh, the code coverage failed again. |
@HerraHak don't worry about the CodeCoverage, that is not blocking. |
@@ -135,7 +135,7 @@ private static VersionInfo GetVersionInfo() | |||
/// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.</param> | |||
/// <param name="imageWidth">Width of resized image.</param> | |||
/// <param name="imageHeight">Height of resized image.</param> | |||
/// <param name="inputColumnName">Name of the input column.</param> | |||
/// <param name="inputColumnName">The name of the input column.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inputColumnName [](start = 25, length = 15)
inputColumnName [](start = 25, length = 15)
/// Name of the column to transform. If set to , the value of the will be used as source.
#Closed
@@ -45,7 +45,7 @@ internal LightGbmMulticlassTrainer(IHostEnvironment env, Options options) | |||
/// Initializes a new instance of <see cref="LightGbmMulticlassTrainer"/> | |||
/// </summary> | |||
/// <param name="env">The private instance of <see cref="IHostEnvironment"/>.</param> | |||
/// <param name="labelColumn">The name of The label column.</param> | |||
/// <param name="labelColumn">The name of the label column.</param> | |||
/// <param name="featureColumn">The name of the feature column.</param> | |||
/// <param name="weights">The name for the column containing the initial weight.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name for the column containing the initial weight. [](start = 34, length = 54)
The name for the column containing the initial weight. [](start = 34, length = 54)
/// <param name="weightsColumnName">The name of the optional weights column.</param>
#Closed
Refers to: src/Microsoft.ML.StandardLearners/Standard/Online/OnlineGradientDescent.cs:102 in c94be09. [](commit_id = c94be09, deletion_comment = False) |
6bf8a17
to
9e1b8b4
Compare
/// <param name="weights">The optional weights column.</param> | ||
/// <param name="labelColumn">The name of the label column.</param> | ||
/// <param name="featureColumn">The name of the feature column.</param> | ||
/// <param name="weights">The name of the optional weights column.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weights [](start = 25, length = 7)
can you update this one as well, otherwise build is not working #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I meant in whole file. We have strict rules to match parameters names in xml documentation
In reply to: 255319474 [](ancestors = 255319474)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be ok now
/// <param name="inputColumnName">Name of the input column.</param> | ||
/// <param name="outputColumnName">Name of the resulting output column.</param> | ||
/// <param name="inputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.</param> | ||
/// <param name="outputColumnName">Name of column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outputColumnName [](start = 25, length = 16)
you need to switch input and output column. Output column is resulting and input column is name of column to transform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how it suppose to be.
/// <param name="outputColumnName"> Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.</param>
/// <param name="inputColumnName"> Name of column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>
And now check what do you have in param name=
In reply to: 255809779 [](ancestors = 255809779)
/// <param name="weights">The weights column.</param> | ||
/// <param name="labelColumn">The name of the label column.</param> | ||
/// <param name="featureColumn">The name of the features column.</param> | ||
/// <param name="weights">The name of the weights column.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the [](start = 46, length = 3)
optional weights column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…m/HerraHak/machinelearning into 2177-update-parameter-description
@abgoswam covered same thing in his PR. |
Well it was nice trying to contribute on this. I'll pick up another issue then. |
Fixes #2177
The description of the parameters have been updated as described by @sfilipi in issue #2177
We are excited to review your PR.
So we can do the best job, please check:
Fixes #nnnn
in your description to cause GitHub to automatically close the issue(s) when your PR is merged.