Skip to content

Adding a test to show supported data types in TensorFlowTransform #2101

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

Jump to bottom
Merged
merged 6 commits into from
Jan 16, 2019
Merged

Adding a test to show supported data types in TensorFlowTransform #2101

merged 6 commits into from
Jan 16, 2019

Conversation

zeahmed
Copy link
Contributor

@zeahmed zeahmed commented Jan 10, 2019

This PR adds addition test to fix #745.

wschin
wschin reviewed Jan 10, 2019
View reviewed changes
test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs Outdated
var mlContext = new MLContext(seed: 1, conc: 1);
// Pipeline
var loader = ComponentCreation.CreateDataView(mlContext,
new List<TestDataInt>(new TestDataInt[] { new TestDataInt() { a = new[] { 1L, 2,
Copy link
Member

@wschin wschin Jan 10, 2019 •
edited by zeahmed
Loading

Choose a reason for hiding this comment

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

Not aligned? #Resolved

yaeldekel
yaeldekel reviewed Jan 10, 2019
View reviewed changes
test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs Outdated
Assert.Equal(4 + 4, cValues[3]);

Assert.True(cursor.MoveNext());
c = default;
Copy link

@yaeldekel yaeldekel Jan 10, 2019 •
edited by zeahmed
Loading

Choose a reason for hiding this comment

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

c = default; [](start = 16, length = 12)

This line is not needed. #Resolved

yaeldekel
yaeldekel reviewed Jan 10, 2019
View reviewed changes
test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs Outdated
VBuffer<long> c = default;
cgetter(ref c);

var cValues = c.GetValues();
Copy link

@yaeldekel yaeldekel Jan 10, 2019 •
edited by zeahmed
Loading

Choose a reason for hiding this comment

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

cValues [](start = 20, length = 7)

Add an assert for the length of this. #Resolved

yaeldekel
yaeldekel reviewed Jan 10, 2019
View reviewed changes
test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs Outdated
c = default;
cgetter(ref c);

cValues = c.GetValues();
Copy link

@yaeldekel yaeldekel Jan 10, 2019 •
edited by zeahmed
Loading

Choose a reason for hiding this comment

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

cValues [](start = 16, length = 7)

Add an assert for the length of this. #Resolved

singlis
singlis reviewed Jan 11, 2019
View reviewed changes
test/Microsoft.ML.Tests/ScenariosWithDirectInstantiation/TensorflowTests.cs Outdated
/// Test to ensure the support for int and int64 types.
/// </summary>
[ConditionalFact(typeof(Environment), nameof(Environment.Is64BitProcess))] // TensorFlow is 64-bit only
public void TensorFlowTransformAdditionOfIntAndInt64Test()
Copy link
Member

@singlis singlis Jan 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

TensorFlowTransformAdditionOfIntAndInt64Test [](start = 20, length = 44)

Hi Zeeshan, is there supposed to be a test for the 32-bit model? I thought the other tests were 32-bit models, but It looks like the other tests either are skipped or disable unless its a 64-bit process.
#Resolved

Copy link
Contributor Author

@zeahmed zeahmed Jan 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

TensorFlow works only on 64-bit platform. So, i think none of the TF tests are 32-bit.

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

Copy link
Member

Choose a reason for hiding this comment

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

ah! sorry I misread, you are handling int64 and int32 types...so then do the other tests use models that are using int32s?

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

Copy link
Member

Choose a reason for hiding this comment

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

actually I see -- you have a TestDataInt class that has both long and int....

In reply to: 246983378 [](ancestors = 246983378,246982838)

singlis
singlis approved these changes Jan 11, 2019
View reviewed changes
Copy link
Member

@singlis singlis left a comment

Choose a reason for hiding this comment

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

:shipit:

@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Jan 11, 2019 •
edited by zeahmed
Loading

Tests are good, but original issue is not about tests, and more about, what types we can actually support?
Can we support bool? Can we support short?
Do we have list of supported types in our documentation?
Do we have any exceptions in case if user passes type which we don't support? Not the cryptic one, but one which would say: "Current type "A" is unsupported please use following types: float, int, int64"? #Resolved

@zeahmed
Copy link
Contributor Author

zeahmed commented Jan 11, 2019

documentation is here:

machinelearning/src/Microsoft.ML.TensorFlow/doc.xml

Line 51 in 447e94e

<description>Currently, float, double, int, long, uint, ulong are the acceptable data types for input/output.</description>

I believe short is also supported. I will add it to the documentation.
Its tedious to write tests for all the types so I created a tests for int/int64 to test for the most common types. But I will see if I can create a generic model that can used to test all the supported types.
I will verify if exceptions are properly thrown. I think if there is a mismatch between the type and type is not supported an exception is thrown.

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

@wschin
Copy link
Member

wschin commented Jan 11, 2019 •
edited by zeahmed
Loading

Can Tensorflow transform work with key types? They are integers as well. #Resolved

@zeahmed zeahmed changed the title Adding a test to show TensorFlowTransform can work with int32 and int64 types Adding a test to show supported data types in TensorFlowTransform Jan 14, 2019
@zeahmed
Copy link
Contributor Author

zeahmed commented Jan 14, 2019

Updated the test to show how the supported types can be used.

In reply to: 453611413 [](ancestors = 453611413,453388399)

@zeahmed
Copy link
Contributor Author

zeahmed commented Jan 14, 2019

No, if they can be represented as any of the these types (double, float, long, int, short, sbyte, ulong, uint, ushort, byte and bool) then yes.

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

Ivanidzo4ka
Ivanidzo4ka approved these changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@zeahmed zeahmed merged commit 68841ad into dotnet:master Jan 16, 2019
@zeahmed zeahmed deleted the tf_int_test branch January 24, 2019 19:14
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wschin wschin wschin left review comments

@yaeldekel yaeldekel yaeldekel left review comments

@Ivanidzo4ka Ivanidzo4ka Ivanidzo4ka approved these changes

@singlis singlis singlis approved these changes

@abgoswam abgoswam Awaiting requested review from abgoswam

@sfilipi sfilipi Awaiting requested review from sfilipi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle additional types as input and output in TF models
5 participants
@zeahmed @Ivanidzo4ka @wschin @yaeldekel @singlis