Skip to content

TextFeaturizingEstimator shouldn't use Argument class as main way of setting itself. #2026

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

Closed
Ivanidzo4ka opened this issue Jan 4, 2019 · 3 comments
Assignees
Labels
API Issues pertaining the friendly API

Comments

@Ivanidzo4ka
Copy link
Contributor

// REVIEW: expose them once sub-transforms are estimators.

Currently if I want to play with Word extractor and Char extractor I need to do that via

public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView data)

which stops me from hiding IDataTransform interface.
We should have proper way to set it up via Setting object.

Related to #1995

@yaeldekel yaeldekel added the API Issues pertaining the friendly API label Jan 4, 2019
@TomFinley
Copy link
Contributor

Yes, we should. Though, that Create mostly exists for dependency injection via component factories, entry-points, and whatnot. So that can totally be private. We also can keep the associated fields on the settings object internal, so we can add a way to configure them via estimators (as described in the comments) elsewhere, I think? What do you think? Good, bad?

@artidoro
Copy link
Contributor

artidoro commented Feb 5, 2019

From API standpoint, TextFeaturizingEstimator is fine once #2394 goes in. However, the entire class should be cleaned up.

@artidoro
Copy link
Contributor

@Ivanidzo4ka after #2394, it seems that this issue has been resolved. If so I will close it.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
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

No branches or pull requests

4 participants