Hide much infrastructure in data by TomFinley · Pull Request #2300 · dotnet/machinelearning

TomFinley · 2019-01-29T01:41:55Z

Another of many steps towards #1602. Commits logically structured. No overall theme, just lots of hiding of individual components, most notably command line parsing, entry-point declarations, and other such things.

codecov · 2019-01-29T02:19:11Z

Codecov Report

Merging #2300 into master will increase coverage by <.01%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #2300      +/-   ##
==========================================
+ Coverage   71.14%   71.14%   +<.01%     
==========================================
  Files         779      779              
  Lines      140243   140251       +8     
  Branches    16024    16024              
==========================================
+ Hits        99773    99783      +10     
+ Misses      36038    36035       -3     
- Partials     4432     4433       +1

Flag	Coverage Δ
#Debug	`71.14% <94.44%> (ø)`	⬆️
#production	`67.57% <94.44%> (ø)`	⬆️
#test	`85.05% <ø> (ø)`	⬆️

sfilipi · 2019-01-29T06:25:35Z

    /// </summary>
-    public sealed class GenerateNumberTransform : RowToRowTransformBase
+    [BestFriend]
+    internal sealed class GenerateNumberTransform : RowToRowTransformBase


internal [](start = 4, length = 8)

why not public?

Pursuant to #1995, all IDataTransforms (as opposed to IEstimator/ITransformers) are to be hidden. So when hiding the entry-point exposing this class, I happened to opportunistically hide this as well, while I was at it.

sfilipi · 2019-01-29T06:34:38Z

    public const string TakeFilterShortName = "Take";

hurts my eyes :)

Refers to: src/Microsoft.ML.Data/Transforms/SkipTakeFilter.cs:48 in 3ac69ce. [](commit_id = 3ac69ce, deletion_comment = False)

codemzs · 2019-01-29T06:35:17Z

Can you please sync to the latest in master because I’m really curious what the new code coverage is?

sfilipi

TomFinley · 2019-01-29T16:42:24Z

Can you please sync to the latest in master because I’m really curious what the new code coverage is?

If it naturally comes up due to an actual required change I'd happily rebase again to master, as I always do. In the absence of any requested changes, I see no particular urgency. This might not be the best PR if you're curious about changes in code coverage, since I did not change any tests, or any infrastructure really. I just made things internal.

If you are curious about this, you can always of course check out my branch yourself and run the coverage numbers locally. In principle it shouldn't change much, since I did not change much infrastructure, just made it internal. (But still according to the report in its current form, I have a +3.29% change in coverage. Who can account for such things.)

I've seen lots of strange stuff like that. Are we certain those code coverage numbers are reliable?

TomFinley · 2019-01-29T17:46:05Z

hurts my eyes :)

The goggles do nothing!

Ivanidzo4ka · 2019-01-29T18:22:01Z

@@ -0,0 +1,137 @@
+Data.CustomTextLoader	Import a dataset from a text file	Microsoft.ML.EntryPoints.ImportTextData	ImportText	Microsoft.ML.EntryPoints.ImportTextData+Input	Microsoft.ML.EntryPoints.ImportTextData+Output


CustomTextLoader [](start = 5, length = 16)

Why we need this file?

We don't, sorry. 😄 I'll remove it. WinMerge keeps creating these stupid files...

shauheen

…ints.

* Parse/TryUnparse methods internalized. * Lockdown of column abstract class constructors to make them unextensible.

…umns.

…ry-points.

…llout.

Ivanidzo4ka

eerhardt · 2019-01-29T18:39:18Z

[BestFriend]

what does BestFriend on a public class mean?

Refers to: src/Microsoft.ML.HalLearners/OlsLinearRegression.cs:38 in 90096c7. [](commit_id = 90096c7, deletion_comment = False)

eerhardt · 2019-01-29T18:42:07Z

Did you mean to check this core_ep-list.tsv.bak file in?

eerhardt · 2019-01-29T18:43:58Z

        /// <summary>
-        /// The data to be used for training.
+        /// The data to be used for training. Used only in entry-points, since in the API the expected mechanism is
+        /// that the user iwll use the <see cref="IEstimator{TTransformer}.Fit(IDataView)"/> or some other train


typeo- iwll

Did you mean to check this core_ep-list.tsv.bak file in?

No. @Ivanidzo4ka had the same comment, I think if you refresh the view you're using you should note that it is removed.

typeo- iwll

Whoops. Since internal only I will fix in next phase, if that's OK.

TomFinley · 2019-01-29T18:56:51Z

[BestFriend]
what does BestFriend on a public class mean?

Refers to: src/Microsoft.ML.HalLearners/OlsLinearRegression.cs:38 in 90096c7. [](commit_id = 90096c7, deletion_comment = False)

It means I'm silly. I'll fix that in next pass.

TomFinley added the API Issues pertaining the friendly API label Jan 29, 2019

TomFinley requested review from Ivanidzo4ka, eerhardt, sfilipi and shauheen January 29, 2019 01:41

TomFinley mentioned this pull request Jan 29, 2019

Make entrypoints inside Microsoft.ML.Transforms project internal #2288

Closed

sfilipi reviewed Jan 29, 2019

View reviewed changes

dotnet deleted a comment from codemzs Jan 29, 2019

sfilipi approved these changes Jan 29, 2019

View reviewed changes

shauheen added this to the 0119 milestone Jan 29, 2019

Ivanidzo4ka reviewed Jan 29, 2019

View reviewed changes

shauheen approved these changes Jan 29, 2019

View reviewed changes

TomFinley added 9 commits January 29, 2019 10:30

Internalize normalizer infrastructure, normalizer transform, entry-po…

0f4cd3b

…ints.

Internalization of component level command line infrastructure.

d31ebaa

* Parse/TryUnparse methods internalized. * Lockdown of column abstract class constructors to make them unextensible.

Removal of legacy API derived interfaces and abstract classes for col…

9224b49

…umns.

Internalization of column bindings utilities and infrastructure.

5629ce0

Hide filtering/misc IDataTransform implementations and associated ent…

e799202

…ry-points.

Internalize or move much of entry-points.

dd9434f

Internalize common outputs of entry-points and deal with resulting fa…

3905ccf

…llout.

Internalization of common inputs.

2e918d9

Internalization of more model saving infrastructure.

3c7820a

TomFinley force-pushed the Hide1 branch from 3ac69ce to 3c7820a Compare January 29, 2019 18:33

Ivanidzo4ka approved these changes Jan 29, 2019

View reviewed changes

eerhardt reviewed Jan 29, 2019

View reviewed changes

TomFinley merged commit 3dd3a1e into dotnet:master Jan 29, 2019

shmoradims mentioned this pull request Jan 29, 2019

Docs rendered incorrectly #1725

Closed

TomFinley mentioned this pull request Jan 29, 2019

Add a Functional.Tests project that doesn't have InternalsVisibleTo #2306

Closed

artidoro mentioned this pull request Jan 29, 2019

Making IDataView TrainingData inside LearnerInputBase internal. #2079

Closed

TomFinley deleted the Hide1 branch February 11, 2019 20:21

TomFinley mentioned this pull request Mar 5, 2019

Hide more of Microsoft.ML.Data #2842

Merged

ghost locked as resolved and limited conversation to collaborators Mar 25, 2022

		@@ -0,0 +1,137 @@
		Data.CustomTextLoader Import a dataset from a text file Microsoft.ML.EntryPoints.ImportTextData ImportText Microsoft.ML.EntryPoints.ImportTextData+Input Microsoft.ML.EntryPoints.ImportTextData+Output

Uh oh!

Conversation

TomFinley commented Jan 29, 2019

Uh oh!

codecov Bot commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sfilipi Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

TomFinley Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

sfilipi commented Jan 29, 2019

Uh oh!

codemzs commented Jan 29, 2019

Uh oh!

sfilipi left a comment

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley commented Jan 29, 2019

Uh oh!

Ivanidzo4ka Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

TomFinley Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shauheen left a comment

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Jan 29, 2019

Uh oh!

eerhardt commented Jan 29, 2019

Uh oh!

eerhardt Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

TomFinley Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

TomFinley Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jan 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Jan 29, 2019 •

edited

Loading

TomFinley commented Jan 29, 2019 •

edited

Loading

TomFinley Jan 29, 2019 •

edited

Loading