-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add public generic methods to TextLoader catalog that accept Options objects #5134
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5134 +/- ##
==========================================
- Coverage 75.83% 75.60% -0.24%
==========================================
Files 951 990 +39
Lines 172613 179227 +6614
Branches 18632 19292 +660
==========================================
+ Hits 130904 135497 +4593
- Misses 36529 38462 +1933
- Partials 5180 5268 +88
|
@@ -1453,6 +1453,23 @@ void ICanSaveModel.Save(ModelSaveContext ctx) | |||
bool trimWhitespace = Defaults.TrimWhitespace, | |||
IMultiStreamSource dataSample = null) | |||
{ | |||
Options options = new Options | |||
{ |
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.
Is the plan to add readMultiLines as an option in a later PR?
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.
readMultilines
is already added to the TextLoader.Options class in my other PR, and it will always default as false, if its value isn't provided.
The method here, that you've pointed to, is used by this public method in the catalog. I can't add a new readMultilines parameter there precisely because it would be a breaking API change. So it doesn't make much sense to me to add a readMultilines option here, and so TextLoader.Options will simply use the default for it (which is false).
In the future, if a user (or we internally) want to use the new options (such as readMultiline), we would simply use a TextLoader.Options object and the overloads for creating TextLoaders that uses the option object. I think it's better and easier to maintain and update only the options object than an overload with several parameters that can't be changed in the public API.
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.
The
TextLoaderSaverCatalog
contains 2 overloads of non-genericCreateTextLoader
andLoadFromTextFile
; one overload accepts a given set of a parameters and the other overload accepts a TexLoader.Options object.The generic methods for
CreateTextLoader<TInput>
andLoadFromTextFile<TInput>
only have 1 method that accepts a given set of parameters. So in this PR I add another overload for those 2 methods that also accepts an Options object.The reason to do this is that if we add new parameters to TextLoader (for example, as done in #5125) then they're only accessible through a TextLoader.Options (so without this PR
CreateTextLoader<TInput>
andLoadFromTextFile<TInput>
won't be able to use the new options). Notice that we can't simply add a new parameter to the existingCreateTextLoader<TInput>
andLoadFromTextFile<TInput>
methods, since that is considered a breaking API change, and we can't do those until ML.NET version 2.