Skip to content

The curious case of TrainedWrapperEstimatorBase and friends #2841

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

Open
TomFinley opened this issue Mar 4, 2019 · 0 comments
Open

The curious case of TrainedWrapperEstimatorBase and friends #2841

TomFinley opened this issue Mar 4, 2019 · 0 comments
Labels
code-sanitation Code consistency, maintainability, and best practices, moreso than any public API. need info This issue needs more info before triage P2 Priority of the issue for triage purpose: Needs to be fixed at some point.

Comments

@TomFinley
Copy link
Contributor

So, while I was doing another round of internalization, one thing that I internalized was this. (Basically, something to handle the shimming from the now internal IDataTransform interface to the new ITransformer interface, during the regrettable situations -- thankfully few -- where such a thing is still necessary. So this:

public sealed class TransformWrapper : ITransformer

and this

public abstract class TrainedWrapperEstimatorBase : IEstimator<TransformWrapper>

Now, that's all fine, but after doing the necessary work it seemed that I could delete the estimator wrapper entirely, but then I see this very intriguing note.

/// <summary>
/// DO NOT USE IT!
/// Purpose of this method is to enable legacy loading and unwrapping of RowToRowTransform.
/// It should be removed as soon as we get rid of <see cref="TrainedWrapperEstimatorBase"/>
/// Returns parent transfomer which uses this mapper.
/// </summary>
ITransformer GetTransformer();

I do not understand what is going on here. This is not essential -- everything here is internal -- but it seems at least odd. This seems to indicate that this method and the class I wanted to delete have something to do with each other, but as far as I can tell they have nothing to do with each other whatsoever. But then why the comment?

Anyway, there's clearly something odd going on. This is absolutely not critical, but I wanted to register an issue about the oddness, since the code underlying it passes my understanding.

@TomFinley TomFinley added need info This issue needs more info before triage code-sanitation Code consistency, maintainability, and best practices, moreso than any public API. labels Mar 4, 2019
@ganik ganik added the P2 Priority of the issue for triage purpose: Needs to be fixed at some point. label May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-sanitation Code consistency, maintainability, and best practices, moreso than any public API. need info This issue needs more info before triage P2 Priority of the issue for triage purpose: Needs to be fixed at some point.
Projects
None yet
Development

No branches or pull requests

2 participants