LightGBM, Tweedie, and GAM trainers converted to tainerEstimators #962
Conversation
Utility methods for the column shape creations.
sfilipi
requested review from
codemzs,
dotnet-bot,
Zruty0,
TomFinley,
zeahmed,
artidoro and
Ivanidzo4ka
|
Usings should be sorted, but code comments coming on the next iteration. #Resolved |
|
Add tests In reply to: 423047158 [](ancestors = 423047158) |
| /// <summary> | ||
| /// The <see cref="SchemaShape.Column"/> for the feature column. | ||
| /// </summary> | ||
| /// <param name="weightColumn">name of the feature column</param> |
There was a problem hiding this comment.
name of the feature column [](start = 39, length = 26)
Description does not agree to name of the parameter...:) #Resolved
There was a problem hiding this comment.
Also, I see that some of the methods have comments while others don't.
In reply to: 219253274 [](ancestors = 219253274)
| private protected readonly Dictionary<string, object> Options; | ||
| private protected readonly IParallel ParallelTraining; | ||
| private protected Dictionary<string, object> Options; | ||
| private protected IParallel ParallelTraining; |
There was a problem hiding this comment.
It would be nice to keep them readonly. I see the problem why you made them non-readonly.
May be call the second constructor from the first one and create Args in static method for passing into the second one.
This way you can initialize them in the second constructor only. #WontFix
There was a problem hiding this comment.
I like the idea, and looked into, but I can't do it because the two constructor call different base class constructors. Merging the constructors earlier is possible, but i feel liek it would look messy, because the distinction on whether you came from the API, or maml would be based on checking presence/absence of parameters.. think I'll leave it as is.
In reply to: 219271083 [](ancestors = 219271083)
| } | ||
|
|
||
| public static SchemaShape.Column MakeU4ScalarLabel(string labelColumn) | ||
| { |
There was a problem hiding this comment.
{ [](start = 7, length = 2)
use => #Resolved
| using Microsoft.ML.Runtime.Data; | ||
| using Microsoft.ML.Runtime.Internal.Utilities; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using Float = System.Single; |
There was a problem hiding this comment.
Float = System.Single [](start = 5, length = 22)
get rid of this altogether in favor of float #Resolved
|
+1, let's have a In reply to: 423051710 [](ancestors = 423051710,423047158) |
ghost
locked as resolved and limited conversation to collaborators
No description provided.