Skip to content
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

Internalize concepts of IDataTransform/Loader/TransformTemplate. #1995

Closed
TomFinley opened this issue Jan 2, 2019 · 1 comment · Fixed by #2509
Closed

Internalize concepts of IDataTransform/Loader/TransformTemplate. #1995

TomFinley opened this issue Jan 2, 2019 · 1 comment · Fixed by #2509
Assignees
Labels
API Issues pertaining the friendly API

Comments

@TomFinley
Copy link
Contributor

Before we had the IEstimator/ITransformer/IDataView triad (see #581), we combined all three concepts inside IDataTransform. The initialization, the data, and the data model were all conflated. This was the source for years of constant confusion internally. We of course had to abandon the idea when we were trying to make an API. Here's the interface, for reference:

public interface IDataTransform : IDataView, ICanSaveModel

Similarly, before we had IDataReaderEstimator/IDataReader/IDataView triad, we combined all three concepts inside IDataLoader.

public interface IDataLoader : IDataView, ICanSaveModel

We also have the old concept of ITransformTemplate, which the ITransformer interface has likewise obviated.

We've introduced the new concepts over the course of the last half year or so, which is great, yet the artifacts of the old world remain. The interfaces should be made internal so people don't stumble into using them. Further, the objects used through ITransform and IDataView should (even when still backed by an IDataTransform object!) be hidden so it is not obvious these concepts are being conflated internally, even if "under the hood" there is just a small shim to present what is (internally) an IDataTransform as publicly an ITransformer/IDataView. (Always with the aim, of course, of having the architecture reflect the shape it presents as its public surface, but what we can refactor over some time we can at least hide now.)

@TomFinley
Copy link
Contributor Author

One example of this that I ran across is the PredictionTransformerBase:

public abstract class PredictionTransformerBase<TModel, TScorer> : IPredictionTransformer<TModel>
where TScorer : RowToRowScorerBase
where TModel : class, IPredictor

This is a Transformer, which is fine, but it quite inappropriately takes as a generic parameter something that is TScorer : RowToRowScorerBase, which is something exposing the "old idioms." However there is no reason to make this detail of implementation public, but it must be as a generic parameter.

In this case, happily, it looks like making this a generic parameter was completely pointless, so it can just be removed and the base class of the scorer used directly (but internally!!) in the abstract base class. But it is at least an interesting example of how badly we started "mixing our metaphors" when the new idioms of #581 were being introduced.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants