Skip to content

Conversation

@aslotte
Copy link
Contributor

@aslotte aslotte commented May 2, 2020

Fixes #2642

This PR reads the KeyTypeAttribute from the schema of the incoming type and sets that on the corresponding column when loading a text file using the CreateTextLoader.

@aslotte aslotte requested a review from a team as a code owner May 2, 2020 15:16
@codecov
Copy link

codecov bot commented May 2, 2020

Codecov Report

Merging #5082 into master will decrease coverage by 0.01%.
The diff coverage is 82.60%.

@@            Coverage Diff             @@
##           master    #5082      +/-   ##
==========================================
- Coverage   75.66%   75.65%   -0.02%     
==========================================
  Files         993      993              
  Lines      178157   178180      +23     
  Branches    19125    19126       +1     
==========================================
- Hits       134800   134795       -5     
- Misses      38136    38158      +22     
- Partials     5221     5227       +6     
Flag Coverage Δ
#Debug 75.65% <82.60%> (-0.02%) ⬇️
#production 71.61% <100.00%> (-0.02%) ⬇️
#test 88.67% <77.77%> (-0.01%) ⬇️
Impacted Files Coverage Δ
test/Microsoft.ML.Tests/TextLoaderTests.cs 95.53% <77.77%> (-0.55%) ⬇️
.../Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs 82.21% <100.00%> (+0.11%) ⬆️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs 79.48% <0.00%> (-20.52%) ⬇️
....ML.AutoML/PipelineSuggesters/PipelineSuggester.cs 83.19% <0.00%> (-4.21%) ⬇️
...icrosoft.ML.AutoML/Experiment/SuggestedPipeline.cs 88.65% <0.00%> (-4.13%) ⬇️
...L.AutoML/TrainerExtensions/TrainerExtensionUtil.cs 85.15% <0.00%> (-1.75%) ⬇️
src/Microsoft.ML.Maml/MAML.cs 24.75% <0.00%> (-1.46%) ⬇️
src/Microsoft.ML.AutoML/Sweepers/Parameters.cs 84.32% <0.00%> (-0.85%) ⬇️
src/Microsoft.ML.Sweeper/AsyncSweeper.cs 73.97% <0.00%> (+1.36%) ⬆️


var data = mlContext.Data.CreateTextLoader<BreastCancerInputModelWithKeyType>(separatorChar: ',').Load(breastCancerPath);

Assert.True(data.Schema[1].Type.GetKeyCount() == 10);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of Assert.True(something == expected), it is better to use Assert.Equal(expected, actual). The reason is when it fails, xunit will print what the expected and actual values are, which helps figure out the issue, without having to re-run the test and debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point @eerhardt, thanks for the review. Pushed the recommended change.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice work on the fix and thanks for the contribution, @aslotte!

@harishsk harishsk merged commit c7b8e87 into dotnet:master May 5, 2020
@harishsk
Copy link
Contributor

harishsk commented May 5, 2020

@aslotte Thank you for the contribution. I have merged your PR.

@aslotte
Copy link
Contributor Author

aslotte commented May 5, 2020

Thanks all!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ReadFromTextFile cannot load data as KeyType

3 participants