Update MacroUtils to map trainer kinds to the correct suffix of trainer entry point names #113
Conversation
| if (type.Name == "BinaryLogisticRegressor") | ||
| return trainerKind == TrainerKinds.SignatureBinaryClassifierTrainer; | ||
| if (type.Name == "LogisticRegressor") | ||
| return trainerKind == TrainerKinds.SignatureMultiClassClassifierTrainer; |
There was a problem hiding this comment.
God, this is so ugly.
Is there a way to cast EntryPoint type to real type? Maybe you can use EntryPointCatalog to find mapping, and after you get real type you can maybe get prediction type, or get kind through reflection?
This is unmaintainable.
There was a problem hiding this comment.
A better solution might be harder to imagine though. We might let this slide for now since this change will fix things that are broken now, but we ought definitely to file a new issue so we can imagine something a bit less fragile than this system.
In reply to: 187421786 [](ancestors = 187421786)
There was a problem hiding this comment.
Could this be addressed when we change the mechanism to use the EntryPoint attribute?
There was a problem hiding this comment.
The entry-point attribute may indeed be the best place to put a category like this -- something akin to the old-style PredictionKind enum, but not that please. :D :D :D
In reply to: 187457279 [](ancestors = 187457279)
|
|
||
| public static bool IsTrainerOfKind(Type type, TrainerKinds trainerKind) | ||
| { | ||
| if (type.Name == "BinaryLogisticRegressor") |
There was a problem hiding this comment.
type.Name == "BinaryLogisticRegressor" [](start = 16, length = 38)
All of these string operations make me nervous.
Is it at all reasonable to change this (and other) type operations to just be type == typeof(BinaryLogisticRegressor) or at least rely on some other reference?
There was a problem hiding this comment.
Is there also any way we can write a test for this, so we can detect when something like this happens again?
In reply to: 187422829 [](ancestors = 187422829)
| @@ -119,7 +119,7 @@ private sealed class TaskInformationBundle | |||
| { | |||
| TrainerKinds.SignatureMultiOutputRegressorTrainer, | |||
| new TaskInformationBundle { | |||
| TrainerFunctionName = "TrainMultiRegression", | |||
| TrainerFunctionName = "MultiOutputRegressor", | |||
There was a problem hiding this comment.
MultiOutputRegressor [](start = 47, length = 20)
You said something about a name suffix... is it a concern that the suffixes are not unique? So Regressor above would be a suffix of any MultiOutputRegressor. We previously worked around this problem I guess by having every suffix start with Train, but that solution has gone out the window I suppose.
There was a problem hiding this comment.
There was a problem hiding this comment.
OK now I know you've special cased this. :) Hmmm I'd still rather have a different solution.
In reply to: 187424194 [](ancestors = 187424194,187424111)
There was a problem hiding this comment.
Thanks @yaeldekel for fixing the problem... let's think more perhaps in an issue about how best to address the architectural deficiencies of this overall scheme.
…ible to EntryPointCatalog test.
…er entry point names (dotnet#113) * Update suffix of trainer entry point names by trainer kind. * Address PR comments. * Add unit test. * Update C# API * Move unit test to TestAutoInference and fix EntryPointCatalog test. * Trigger build. * Add reference to the test project to make the sweeper entry point visible to EntryPointCatalog test.
ghost
locked as resolved and limited conversation to collaborators
Closes #112 .
Entry points for binary classifiers end with "BinaryClassifier", for regressors end with "Regressor", etc. Update the mapping in MacroUtils to enable finding the entry points of trainers for a specific task.