-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Removed ability to save filters from legacy filter code #5338
Conversation
@@ -219,32 +174,12 @@ private static ITransformer Create(IHostEnvironment env, ModelLoadContext ctx) | |||
/// </summary> | |||
internal abstract class LambdaTransformBase : ICanSaveModel |
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.
ICanSaveModel [](start = 50, length = 13)
Since this doesn't need to implement ICanSaveModel
, and there is no other functionality in this base class, I would delete it. You mentioned that it is being used internally only as an IDataView
, in that case I would make the deriving classes (I only found one, StatefulFilterTransform
) implement IDataView
instead of ITransformTemplate
. #Resolved
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.
Thanks @yaeldekel. I have deleted LambdaTransformBase
. I replaced ITransformTemplate
with IDataTransform
instead of IDataView
because SequentialTransformBase
in the time series code was relying on it being IDataTransform
In reply to: 466166529 [](ancestors = 466166529)
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.
Never mind. Your advice was correct. The earlier fix resulted in test failures. I have now changed StatefulFilterTransform
to implement IDataView
In reply to: 466589874 [](ancestors = 466589874,466166529)
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.
LGTM
Just to mention that this also is meant to fix CA2301 on #5334 😄 |
Codecov Report
@@ Coverage Diff @@
## master #5338 +/- ##
==========================================
- Coverage 73.92% 69.84% -4.08%
==========================================
Files 1020 775 -245
Lines 190126 145190 -44936
Branches 20437 18529 -1908
==========================================
- Hits 140549 101414 -39135
+ Misses 44048 38611 -5437
+ Partials 5529 5165 -364
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Fixes #5335
The offending code in ResultProcessor wasn't being used at all. So I have deleted that.
And as per @yaeldekel the StatefulFilterTransform is a legacy filter from TLC and is not part of the ML.NET public API. It is being used internally in some time series code. But it doesn't need the ability to be saved.
We should at some point, upgrade the time series code to use the new CustomMappingTransform and delete the StatefulFilterTransform.
Until then I have deleted the SerializableLambdaTransform and the associated functionality in LambdaTransformBase.