-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Lockdown Microsoft.ML.StandardLearners public surface #2541
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
Conversation
New API list: N:Microsoft.ML #Resolved |
Codecov Report
@@ Coverage Diff @@
## master #2541 +/- ##
==========================================
+ Coverage 71.5% 71.5% +<.01%
==========================================
Files 801 801
Lines 142036 142036
Branches 16151 16151
==========================================
+ Hits 101558 101562 +4
+ Misses 36005 36003 -2
+ Partials 4473 4471 -2
|
I would make it internal for now, since user has nothing to do with this object other than inspect it. #Closed Refers to: src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs:36 in 4148907. [](commit_id = 4148907, deletion_comment = False) |
Can we make this class internal instead of making it's methods internal? Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs:998 in 4148907. [](commit_id = 4148907, deletion_comment = False) |
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.
Looks awesome! Thanks for doing such a great job!
We keep all our learners extensions in |
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.
Thank you @codemzs !! This is a major step forward.
Its a partial class and main class for LR which is exposed to the user for LR trainer. I don't think we can make it internal here. In reply to: 463745660 [](ancestors = 463745660) Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs:998 in da21bd5. [](commit_id = da21bd5, deletion_comment = False) |
I think the reason for that is FM has its own folder where it contains the trainer, model params, and catalog. In reply to: 463748675 [](ancestors = 463748675) |
da21bd5
to
4148907
Compare
4148907
to
5286633
Compare
fixes #2264
Reduces public API count from 529 to 303.