transform and trainer namespaces don't need to follow the catalogs #2755
Conversation
sfilipi
requested review from
codemzs,
Ivanidzo4ka,
artidoro,
eerhardt and
TomFinley
| @@ -17,6 +17,7 @@ | |||
| using Microsoft.ML.Internal.Utilities; | |||
| using Microsoft.ML.Model; | |||
| using Microsoft.ML.Trainers.FastTree; | |||
| using Microsoft.ML.Transforms; | |||
There was a problem hiding this comment.
Transforms [](start = 19, length = 10)
I'm seeing a surprising number of these introductions. I don't object, but I am fairly curious what changed that they were no longer necessary in the past but are necessary now. Things like using Microsoft.ML.Transforms.Projections; changing to using Microsoft.ML.Transforms; makes total sense to me, but what happened that a totally novel using became necessary?
This is not an urgent question to be clear. Just idly curious, something to answer in your copious spare time. ;) #Pending
There was a problem hiding this comment.
Good point! It is due to FeatureContributionCalculatingTransformer moving from Microsoft.ML.Data into Microsoft.ML.Transforms.
Ml.Data was already there.
In reply to: 260841129 [](ancestors = 260841129)
| @@ -13,13 +13,13 @@ | |||
| using Microsoft.ML.Model; | |||
| using Microsoft.ML.Model.OnnxConverter; | |||
| using Microsoft.ML.Numeric; | |||
| using Microsoft.ML.Trainers.KMeans; | |||
| using Microsoft.ML.Trainers; | |||
| using Float = System.Single; | |||
|
|
|||
There was a problem hiding this comment.
Ugh. One of you and @jwood803 are going to make each other very unhappy with the rebase conflicts. :D
There was a problem hiding this comment.
I don't mind doing the merge on my pull request. :)
sfilipi
changed the title
| @@ -64,7 +64,7 @@ | |||
| <remarks> | |||
| <para> | |||
| This transform uses a set of aggregators to count the number of non-default values for each slot and | |||
| instantiates a <see cref="SlotsDroppingTransformer"/> to actually drop the slots. | |||
| instantiates a <see cref="T:Microsoft.ML.Transforms.SlotsDroppingTransformer"/> to actually drop the slots. | |||
There was a problem hiding this comment.
Microsoft.ML.Transforms [](start = 38, length = 23)
Similar question, not sure why this suddenly became necessary when previously it was not. Or did we only just realize it was necessary? Totally fine, consistent with everythign else I see here, just a little odd. #Pending
There was a problem hiding this comment.
this was not necessary; spotted the difference when changed the namespaces below, and i guess my OCD took over :)
In reply to: 260843071 [](ancestors = 260843071)
There was a problem hiding this comment.
Looks good @sfilipi thanks for doing this! Note that your EntryPointCatalog test is failing -- you've changed the namespaces of many key transforms, which will likewise change the fully qualified names that appear in the catalog, so you'll have to regenerate that. But probably you've already determined this. Otherwise looks good, thank you again.
| @@ -22,7 +22,7 @@ | |||
|
|
|||
| [assembly: LoadableClass(typeof(void), typeof(FeatureContributionEntryPoint), null, typeof(SignatureEntryPointModule), FeatureContributionCalculatingTransformer.LoaderSignature)] | |||
|
|
|||
| namespace Microsoft.ML.Data | |||
| namespace Microsoft.ML.Transforms | |||
There was a problem hiding this comment.
Note, this is the only class that moved from ML.Data ->ML.Transforms.
Everythign else was a sub-namespace of ML.Transforms or ML.Trainers #WontFix
There was a problem hiding this comment.
Oh geez. That's good that you caught it!
sfilipi
force-pushed
the
trainerTransformNamespaces
branch
from
f60da28
to
62366e9
Compare
Codecov Report
@@ Coverage Diff @@
## master #2755 +/- ##
=======================================
Coverage 71.65% 71.65%
=======================================
Files 807 807
Lines 142337 142337
Branches 16117 16117
=======================================
Hits 101986 101986
+ Misses 35916 35915 -1
- Partials 4435 4436 +1
|
ghost
locked as resolved and limited conversation to collaborators
Microsoft.Ml.Transforms.Normalizers
Microsoft.Ml.Transforms.Categoricals
Microsoft.Ml.Transforms.Conversions
Microsoft.Ml.Transforms.Projections
into Microsoft.Ml.Transforms
Microsoft.ML.Trainers.KMeans
Microsoft.ML.Trainers.PCA
Microsoft.ML.Trainers.OnlineLearners
Microsoft.ML.Trainers.FactorizationMachine
into Microsoft.ML.Trainers
Addresses #2751.