Skip to content

undoing test changes - #974

Merged
sfilipi merged 2 commits into
dotnet:masterfrom
sfilipi:fixTest
Sep 22, 2018
Merged

sfilipi merged 2 commits into
dotnet:masterfrom
sfilipi:fixTest

Conversation

@sfilipi

@sfilipi sfilipi commented Sep 21, 2018

Copy link
Copy Markdown
Member

Fixing merge errors on those two files, on the TreeEstimators PR.

@sfilipi
sfilipi requested review from Zruty0 and artidoro September 21, 2018 05:16
.Append(new Ova(env, sdcaTrainer))
.Append(new KeyToValueEstimator(env, "PredictedLabel"));

var model = pipeline.Fit(data);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to do something with the model to ensure it was made correctly?

@sfilipi sfilipi Sep 21, 2018 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

was thinking the same about all the tests we added recently. I'll so a sweep to all of them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The purpose of these 'tests' is generally to demonstrate how we perform the scenario listed in the summary. I think it's enough that the code compiles and runs without throwing.
There are separate tests for estimators, trainers etc., that are designed to verify that the trained models behave as expected. Making every test validate everything is, I think, not necessary.


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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My thoughts:

Do those separate tests do the exact same thing as these? If yes, then these tests are redundant. If no, then these tests should be validating the scenario works as expected because the separate tests are doing something different.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will compact them. Thanks @eerhardt.

@Zruty0 Zruty0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:


var testLoader = TextLoader.ReadFile(env, MakeIrisTextLoaderArgs(), new MultiFileSource(dataPath));
var testData = testLoader.AsEnumerable<IrisDataNoLabel>(env, false);
var testData = testLoader.AsEnumerable<IrisData>(env, false);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IrisData [](start = 55, length = 8)

@Zruty0 instead?

@markusweimer

markusweimer commented Sep 21, 2018 •

Copy link
Copy Markdown

This PR does not reference an issue. Please fix this before merging. #Resolved

@sfilipi

sfilipi commented Sep 21, 2018

Copy link
Copy Markdown
Member Author

Added: #983


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

@TomFinley TomFinley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing merge issue @sfilipi !

@sfilipi
sfilipi merged commit 1f5f696 into dotnet:master Sep 22, 2018
@sfilipi
sfilipi deleted the fixTest branch September 22, 2018 05:01
@sfilipi sfilipi mentioned this pull request Oct 1, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2022
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.

5 participants