Fix creation of dataviews inferred with .NET types with sparse vectors #587

xadupre · 2018-07-27T08:36:07Z

Fixes #586. DataViewConstructionUtils's methods to create dataviews over .NET types will now have correctly inferred getters in the case of sparse vectors.

…issue dotnet#78

dnfclas · 2018-07-27T08:36:19Z

All CLA requirements met.

TomFinley · 2018-07-27T17:23:23Z

test/Microsoft.ML.TestFramework/TestSparseDataView.cs

+            Done();
+        }
+
+        void GenericDenseDataView<T>(T[] v1, T[] v2)


void [](start = 8, length = 4)

Explicit access modifier please. Here and everywhere.

TomFinley · 2018-07-27T17:23:32Z

test/Microsoft.ML.TestFramework/TestSparseDataView.cs

+
+namespace Microsoft.ML.Runtime.RunTests
+{
+    public sealed partial class TestSparseDataView : TestDataViewBase


partial [](start = 18, length = 7)

Why partial?

Copy paste without thinking :)

TomFinley · 2018-07-27T17:28:16Z

Hi @xadupre thanks for adding this fix plus the extensive tests. Right now the PR description is just the default description. Perhaps something real could be written there. If we were to merge right now it would be very hard to come up with an adequate commit message. (Complicated also by the fact that this PR appears to be based on merges rather than rebases.)

Also if you write something like "fixes #586" in your description, then it will associate the issue and the PR, and close the issue when the PR is merged. You put the thing in the title, but if you check the issue page it was not actually associated. (Well, it will be now since I just mentioned it, but of course as the PR author it should be in the head description, rather than some random reviewer mentioning it in a comment far down the page.

TomFinley · 2018-07-27T17:29:54Z

test/Microsoft.ML.TestFramework/TestSparseDataView.cs

+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.ML.Runtime.Command;


I'm fairly certain not all of these are used.

Ivanidzo4ka · 2018-07-27T17:37:39Z

test/Microsoft.ML.TestFramework/TestSparseDataView.cs

+                new SparseExample<T>() { X = new VBuffer<T> (5, 3, v1, new int[] { 0, 2, 4 }) },
+                new SparseExample<T>() { X = new VBuffer<T> (5, 3, v2, new int[] { 0, 1, 3 }) }
+            };
+            var host = new TlcEnvironment();


var host = new TlcEnvironment(); [](start = 12, length = 32)

can you wrap it in using? #Resolved

Ivanidzo4ka · 2018-07-27T17:38:19Z

test/Microsoft.ML.TestFramework/TestSparseDataView.cs

+            GenericSparseDataView(new double[] { 1, 2, 3 }, new double[] { 1, 10, 100 });
+            GenericSparseDataView(new DvText[] { new DvText("a"), new DvText("b"), new DvText("c") },
+                                  new DvText[] { new DvText("aa"), new DvText("bb"), new DvText("cc") });
+            Done();


Done(); [](start = 12, length = 7)

You don't have to call Done if you don't do any basefiles comparison #Resolved

Ivanidzo4ka · 2018-07-27T17:49:00Z

test/Microsoft.ML.TestFramework/TestSparseDataView.cs

+            Assert.True(n == 2);
+        }
+
+        [Fact]


Fact [](start = 9, length = 4)

Thank you for fixing my mess!
Your code definitely do right thing, and it's nice to have tests, but I want to point one thing. Your tests never hit TypedCursor changes.
There is two conversions: one from some collection to IDataview through code in DataViewConstructionUltis, and from IDataVIew to Collection via TypedCursor and your tests cover only first case.

In order to invoke second case you need to do something like:
dataView.AsEnumerable(env, false).GetEnumerator() and enumerate over it.
#Resolved

I'll do it. #Resolved

Ivanidzo4ka

TomFinley · 2018-07-28T14:58:53Z

Hi @xadupre thanks for addressing the comment! Clarifying note for the future, when I asked to modify the description, I meant the PR's description, not the PR's title. The description is still the little "we are excited to review your PR" built in text that we intend for people to write. I hope you don't mind, just going to change it so we can get this in since otherwise it looks good.

TomFinley

Thanks @xadupre

xadupre · 2018-07-30T08:43:46Z

Hi, Sorry for that. I did not read this part, I just checked the checkboxes. I looked into the template used by scikit-learn and the text I forgot to replace was commented out (HTML comment) and here is the discussion about it (scikit-learn/scikit-learn#6470). That might be a good idea for us too. Xavier Sent from Outlook<http://aka.ms/weboutlook>

…

________________________________ De : Tom Finley <notifications@github.com> Envoyé : samedi 28 juillet 2018 16:59 À : dotnet/machinelearning Cc : Xavier Dupre; Mention Objet : Re: [dotnet/machinelearning] Fixes #586, the creation of getters on dataviews having sparse vectors (#587) Hi @xadupre<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fxadupre&data=02%7C01%7Cxadupre%40microsoft.com%7Cbaa687d82f064276dfc808d5f49aa905%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636683867461378704&sdata=44hUPnHMRw855yU6XeDRnBVJqt5AbS8D47Xo3hpzsMU%3D&reserved=0> thanks for addressing the comment! Clarifying note for the future, when I asked to modify the description, I meant the PR's description, not the PR's title. The description is still the little "we are excited to review your PR" built in text that we intend for people to write. I hope you don't mind, just going to change it so we can get this in since otherwise it looks good. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fmachinelearning%2Fpull%2F587%23issuecomment-408613481&data=02%7C01%7Cxadupre%40microsoft.com%7Cbaa687d82f064276dfc808d5f49aa905%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636683867461378704&sdata=hTqY1KEejcLiOP%2FdPGGrE8PnSXAvHwMMHhQ%2BE8NgQp4%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAVaaLWE3ZD_HevTsQpdsPZCEJWuaEyMOks5uLHw3gaJpZM4VjH0j&data=02%7C01%7Cxadupre%40microsoft.com%7Cbaa687d82f064276dfc808d5f49aa905%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636683867461388711&sdata=jZxqC736MCPKl4JydC0GK8w4F2XjXeR0BxY9eGZwnGo%3D&reserved=0>.

dotnet#587) `DataViewConstructionUtils`'s methods to create dataviews over .NET types will now have correctly inferred "getters" in the case of sparse vectors.

sdpython and others added 14 commits May 18, 2018 19:31

Compile CpuMathNative and FastTreeNative with charset=utf-8, fix for …

edba472

…issue dotnet#78

Merge branch 'master' of https://github.com/dotnet/machinelearning

5df6fba

Merge branch 'master' of https://github.com/dotnet/machinelearning

ca76231

Merge branch 'master' of https://github.com/dotnet/machinelearning

ea730aa

add method AddSerialize to declare entrypoints outside ML.net

95e3646

Merge branch 'master' of https://github.com/dotnet/machinelearning

96cae17

Merge branch 'master' of https://github.com/dotnet/machinelearning

e55882d

Merge branch 'master' of https://github.com/dotnet/machinelearning

719adc4

Merge branch 'master' of https://github.com/dotnet/machinelearning

0255942

Merge branch 'master' of https://github.com/dotnet/machinelearning

5d55dfa

add a unit test to check the creation of a cursor on sparse vectors

d5a419e

Merge branch 'master' of https://github.com/dotnet/machinelearning

11aea96

fix dotnet#586 and cursor creation on dataview with sparse vectors

3409b9f

revert changes not related to dotnet#586

63fa647

TomFinley reviewed Jul 27, 2018

View reviewed changes

Ivanidzo4ka reviewed Jul 27, 2018

View reviewed changes

xadupre changed the title ~~Fix the creation of getters on dataviews having sparse vectors (#586)~~ Fixes #586, the creation of getters on dataviews having sparse vectors Jul 27, 2018

extend coverage of unit tests, addresses PR's comment

bebf262

Ivanidzo4ka approved these changes Jul 27, 2018

View reviewed changes

TomFinley changed the title ~~Fixes #586, the creation of getters on dataviews having sparse vectors~~ Fix creation of dataviews inferred with .NET types with sparse vectors Jul 28, 2018

TomFinley approved these changes Jul 28, 2018

View reviewed changes

TomFinley merged commit 2107b82 into dotnet:master Jul 28, 2018

ghost locked as resolved and limited conversation to collaborators Mar 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix creation of dataviews inferred with .NET types with sparse vectors #587

Fix creation of dataviews inferred with .NET types with sparse vectors #587

Uh oh!

xadupre commented Jul 27, 2018 •

edited by TomFinley

Loading

Uh oh!

dnfclas commented Jul 27, 2018 •

edited

Loading

Uh oh!

TomFinley Jul 27, 2018

Uh oh!

TomFinley Jul 27, 2018

Uh oh!

xadupre Jul 27, 2018

Uh oh!

TomFinley commented Jul 27, 2018

Uh oh!

TomFinley Jul 27, 2018

Uh oh!

Ivanidzo4ka Jul 27, 2018 •

edited

Loading

Uh oh!

Ivanidzo4ka Jul 27, 2018 •

edited

Loading

Uh oh!

Ivanidzo4ka Jul 27, 2018 •

edited

Loading

Uh oh!

xadupre Jul 27, 2018 •

edited by Ivanidzo4ka

Loading

Uh oh!

Ivanidzo4ka left a comment

Uh oh!

TomFinley commented Jul 28, 2018

Uh oh!

TomFinley left a comment

Uh oh!

xadupre commented Jul 30, 2018 via email

Uh oh!

Uh oh!

Fix creation of dataviews inferred with .NET types with sparse vectors #587

Fix creation of dataviews inferred with .NET types with sparse vectors #587

Uh oh!

Conversation

xadupre commented Jul 27, 2018 • edited by TomFinley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

xadupre Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jul 27, 2018

Uh oh!

TomFinley Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xadupre Jul 27, 2018 • edited by Ivanidzo4ka Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jul 28, 2018

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

xadupre commented Jul 30, 2018 via email

Uh oh!

Uh oh!

xadupre commented Jul 27, 2018 •

edited by TomFinley

Loading

dnfclas commented Jul 27, 2018 •

edited

Loading

Ivanidzo4ka Jul 27, 2018 •

edited

Loading

Ivanidzo4ka Jul 27, 2018 •

edited

Loading

Ivanidzo4ka Jul 27, 2018 •

edited

Loading

xadupre Jul 27, 2018 •

edited by Ivanidzo4ka

Loading