-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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
[SPARK-14985][ML] Update LinearRegression, LogisticRegression summary internals to handle model copy #12823
Conversation
Regarding linear regression, should the Same thing for logistic regression, should the different metrics be moved to the training summary? Also, I noticed some discrepancy regarding the visibility of some fields in linear regression compared to generalized linear regression:
Is this intentional? |
Test build #57482 has finished for PR 12823 at commit
|
@BenFradet I'm sorry for dropping the ball on this one. Did you close this due to inactivity? If you're willing, it would be nice to do this cleanup. To answer your questions:
These can go in the summary, not just the training summary, since they can be calculated from the model. The training summary just has values which are specific to the training process.
Making numInstances and degreesOfFreedom public sounds good to me. Thanks! |
@jkbradley I did close this due to inactivity, I'm reopening it as I now have a bit of time. |
Test build #75107 has finished for PR 12823 at commit
|
Test build #75110 has finished for PR 12823 at commit
|
… training summary
Test build #75169 has finished for PR 12823 at commit
|
Test build #75242 has finished for PR 12823 at commit
|
|
Jenkins, retest this please |
Test build #75245 has finished for PR 12823 at commit
|
ping @jkbradley if you could take a look, that'd be great. If you have the time, there is also the #17431 segue. |
@BenFradet I think this needs to be updated now - and should also take into account #15435 and #17586 |
@MLnick ok, I'll have a look |
@MLnick should we wait on those PRs being merged before moving forward with this? |
Test build #95021 has finished for PR 12823 at commit
|
What changes were proposed in this pull request?
The summaries now have a internal copy of the model
How was this patch tested?
Reran the concerned suites