-
Notifications
You must be signed in to change notification settings - Fork 831
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
Adding getBestModel and getBestModelInfo to TuneHyperparametersModel #355
Adding getBestModel and getBestModelInfo to TuneHyperparametersModel #355
Conversation
a96fb41
to
a0f3731
Compare
Pass! — The build has succeeded. (a0f37312) MMLSpark 0.13.dev21+1.ga0f3731This is a build for Github PR #355, changes:
|
b68dc3b
to
7a04ead
Compare
Pass! — The build has succeeded. (7a04ead6) MMLSpark 0.13.dev21+1.g7a04eadThis is a build for Github PR #355, changes:
|
} | ||
val (modelClass, modelQualifiedClass) = sc.getSuperclass.getSimpleName match { | ||
case "Estimator" => getModelFromGenericType(typeArgs.head) | ||
case "ProbabilisticClassifier" => getModelFromGenericType(typeArgs(2)) |
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.
Why do we need to specifically code against this API here? seems like this muddies the abstraction and is not needed becase a Probabbalistic Classifier is a predictor
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 needed probabilistic classifier to get the model type, otherwise in the autogenerated lightgbm wrappers the model was "M" instead of lightgbmclassificationmodel
@@ -23,6 +23,8 @@ abstract class WrapperGenerator { | |||
|
|||
def wrapperName(myClass: Class[_]): String | |||
|
|||
def modelWrapperName(myClass: Class[_], modelName: String): String | |||
|
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.
does this need to also take in the modelName?
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 model wrapper needs to take in the model name because that is what it returns, it uses the class to determine whether to put an underscore at the beginning
@@ -9,7 +9,7 @@ | |||
basestring = str | |||
|
|||
from mmlspark._LightGBMRegressor import _LightGBMRegressor | |||
from mmlspark._LightGBMRegressor import M | |||
from mmlspark._LightGBMRegressor import _LightGBMRegressionModel |
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 nice
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.
thanks
…ixed autogenerated code for model name
7a04ead
to
c1ea31d
Compare
Pass! — The build has succeeded. (c1ea31d1) MMLSpark 0.13.dev23+1.gc1ea31dThis is a build for Github PR #355, changes:
|
Adding getBestModel and getBestModelInfo to TuneHyperparametersModel (python API)