-
Notifications
You must be signed in to change notification settings - Fork 1.9k
WIP End of an era - Delete Microsoft.ML.Legacy and related tests. #1971
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
| thrown = true; | ||
| } | ||
| Assert.True(thrown); | ||
| } |
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.
Revert this change.
| @@ -1,62 +0,0 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <doc> | |||
| <members> | |||
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.
this is still needed. you can swap the references
| namespace Microsoft.ML.Benchmarks | ||
| { | ||
| #pragma warning disable 612, 618 | ||
| public class LegacyPredictionEngineBench |
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.
LegacyPredictionEngineBench [](start = 17, length = 27)
does this need to get translated to the regular prediction engine?
| module SmokeTest1 = | ||
|
|
||
| type SentimentData() = | ||
| [<LoadColumn(columnIndex = 0); DefaultValue>] |
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.
[<LoadColumn(columnIndex = 0); DefaultValue> [](start = 7, length = 45)
i think those need to get translated too.
| namespace Microsoft.ML.Tests | ||
| { | ||
| #pragma warning disable 612, 618 | ||
| public class OnnxTests : BaseTestBaseline |
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.
OnnxTests [](start = 17, length = 9)
are there other ONNX tests? Do those need to migrate?
| using Xunit; | ||
|
|
||
| namespace Microsoft.ML.Scenarios | ||
| { |
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.
I am not positive those tests have an equivalent through the other API.
I know the Scenarios/PipelineApi folder ones do.
| } | ||
|
|
||
| [Fact] | ||
| public void InitializerCreationTest() |
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 this test really related to legacy APIs?
|
closing this since #2043 replaces this PR. |
I still need to remove all those baseline files that are no longer used and then document all the tests that may need to be re-written using the new API such as ONNX converter test and FSharp test. In the end I want to compare how much the code coverage has changed between this change and master.
CC: @TomFinley, @sfilipi