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

Fix ModelInsights for xgboost #170

Merged
merged 5 commits into from
Nov 6, 2018
Merged

Fix ModelInsights for xgboost #170

merged 5 commits into from
Nov 6, 2018

Conversation

kinfaikan
Copy link
Contributor

Related issues
OpWorkflowModel::modelInsights fails for xgboost because feature scores are not extracted correctly

Describe the proposed solution
Convert feature score map to a vector in ModelInsights::getModelInsights
Handle null stage param values in ModelInsights::getStageInfo
Do not create a table when no validation results is found

Describe alternatives you've considered
NA

Additional context
NA

@salesforce-cla
Copy link

salesforce-cla bot commented Nov 2, 2018

Thanks for the contribution! Unfortunately we can't verify the commit author(s): kinfai.kan <k***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

* @param featureVectorSize
* @return
*/
def getFeatureScoreVector(featureVectorSize: Option[Int]): Vector = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. why use Vector as return type for if you are doing to toArray anyways?
  2. should we have a default value featureVectorSize: Option[Int] = None?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Nov 3, 2018

Codecov Report

Merging #170 into master will increase coverage by 0.04%.
The diff coverage is 96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
+ Coverage   86.25%   86.29%   +0.04%     
==========================================
  Files         305      305              
  Lines        9950     9963      +13     
  Branches      319      549     +230     
==========================================
+ Hits         8582     8598      +16     
+ Misses       1368     1365       -3
Impacted Files Coverage Δ
.../ml/dmlc/xgboost4j/scala/spark/XGBoostParams.scala 95% <100%> (+5%) ⬆️
...c/main/scala/com/salesforce/op/ModelInsights.scala 92.24% <93.33%> (+0.5%) ⬆️
...ages/impl/classification/OpXGBoostClassifier.scala 31.57% <0%> (+1.75%) ⬆️
...op/stages/impl/regression/OpXGBoostRegressor.scala 18.18% <0%> (+2.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ad1169...faa267e. Read the comment docs.

@tovbinm tovbinm changed the title fix ModelInsights for xgboost Fix ModelInsights for xgboost Nov 3, 2018
Copy link
Collaborator

@leahmcguire leahmcguire left a comment

Choose a reason for hiding this comment

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

lgtm

@tovbinm tovbinm merged commit 1e17a59 into master Nov 6, 2018
@tovbinm tovbinm deleted the kk/xgboostModelInsights branch November 6, 2018 18:42
ericwayman pushed a commit that referenced this pull request Feb 8, 2019
@salesforce-cla
Copy link

salesforce-cla bot commented Mar 8, 2021

Thanks for the contribution! It looks like @kinfaikan is an internal user so signing the CLA is not required. However, we need to confirm this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants