Skip to content

Code generate TextLoader API and enhance it with convenience API. #142

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.

Sign up for GitHub

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

Merged
merged 24 commits into from
May 22, 2018
Merged

Code generate TextLoader API and enhance it with convenience API. #142

Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
3b88d2f
Code generated loader API and install it at the backend pipeline load…
codemzs May 6, 2018
f46f36a
Merge branch 'master' of https://github.com/dotnet/machinelearning in…
codemzs May 6, 2018
c0ff881
Update baselines and add checks for invalid loader arguments.
codemzs May 6, 2018
6d5f351
Revert test metric changes and change test file for evaluation to be …
codemzs May 6, 2018
51d5658
cleanup.
codemzs May 7, 2018
24495fb
Merge branch 'master' of https://github.com/dotnet/machinelearning in…
codemzs May 7, 2018
628cd1d
Merge branch 'master' of https://github.com/dotnet/machinelearning in…
codemzs May 14, 2018
c5dd614
resolve conflits from upstream/master merge.
codemzs May 14, 2018
a61d244
Feedback from PR#38
codemzs May 14, 2018
847f3f1
PR feedback.
codemzs May 14, 2018
9a6b97c
Merge branch 'master' of https://github.com/dotnet/machinelearning in…
codemzs May 15, 2018
5f153fb
PR feedback.
codemzs May 15, 2018
3b57ae6
PR feedback.
codemzs May 16, 2018
4119b43
PR feedback.
codemzs May 16, 2018
755eb34
PR feedback.
codemzs May 16, 2018
743fa6f
PR feedback.
codemzs May 17, 2018
b5c4fcb
Merge branch 'master' of https://github.com/dotnet/machinelearning in…
codemzs May 17, 2018
bb1afd4
PR feedback.
codemzs May 17, 2018
596079f
PR feedback.
codemzs May 17, 2018
b1d44b3
Merge branch 'master' of https://github.com/dotnet/machinelearning in…
codemzs May 18, 2018
6233b76
PR feedback.
codemzs May 18, 2018
a6805c9
PR feedback.
codemzs May 19, 2018
ca98fc5
Merge branch 'master' of https://github.com/dotnet/machinelearning in…
codemzs May 22, 2018
78e89c3
PR feedback.
codemzs May 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
PR feedback.
  • Loading branch information
@codemzs
codemzs committed May 22, 2018
commit 78e89c3fb51e2bf67fab649e98ae4855ca08acae
4 changes: 2 additions & 2 deletions src/Microsoft.ML/Data/TextLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ public TextLoaderRange(int ordinal)
public TextLoaderRange(int min, int max)
Copy link
Contributor

@TomFinley TomFinley May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<param tags? Isn't it important to explain these? #Closed

{

Contracts.CheckParam(min >= 0, nameof(min), "Cannot be negative number.");
Contracts.CheckParam(max >= min, nameof(max), $"Cannot be less than {nameof(min)}.");
Contracts.CheckParam(min >= 0, nameof(min), "Cannot be a negative number.");
Contracts.CheckParam(max >= min, nameof(max), "Cannot be less than " + nameof(min) +".");

Min = min;
Copy link
Contributor

@veikkoeeva veikkoeeva May 16, 2018 •
edited by codemzs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Negative numbers? Both zero? Both the same? Min larger than max? #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to \src\microsoft.ml.data\dataloadsave\text\textloader.cs line 149 - isValid method

In reply to: 188737311 [](ancestors = 188737311)

Copy link
Contributor

@veikkoeeva veikkoeeva May 17, 2018 •
edited by codemzs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codemzs It's

machinelearning/src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs

Line 149 in c023727

public bool IsValid()
I suppose. While there is this function, it would make sense to check invariants "on the spot" also (if these cases really are not allowed). Now it's not possible to see this invariant here, locally, just by looking at this code and correspondginly have an error "on the spot" for erroneous case. It also creates a precedent to write code that is more brittle in that if something is changed "somebody else somewhere checks for the validity". It's very difficult to anyone outside of the project, or new to project, make any changes without reading a lot of code (which likely means it won't happen). #Resolved

Max = max;
Copy link
Contributor

@TomFinley TomFinley May 16, 2018 •
edited by codemzs
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar for this. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation I mean.

In reply to: 188720207 [](ancestors = 188720207)

Expand Down