Fix creation of dataviews inferred with .NET types with sparse vectors #587
Conversation
|
|
| Done(); | ||
| } | ||
|
|
||
| void GenericDenseDataView<T>(T[] v1, T[] v2) |
There was a problem hiding this comment.
void [](start = 8, length = 4)
Explicit access modifier please. Here and everywhere.
|
|
||
| namespace Microsoft.ML.Runtime.RunTests | ||
| { | ||
| public sealed partial class TestSparseDataView : TestDataViewBase |
There was a problem hiding this comment.
partial [](start = 18, length = 7)
Why partial?
There was a problem hiding this comment.
Copy paste without thinking :)
|
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. |
| using System.Linq; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.ML.Runtime.Command; |
There was a problem hiding this comment.
I'm fairly certain not all of these are used.
| 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(); |
There was a problem hiding this comment.
var host = new TlcEnvironment(); [](start = 12, length = 32)
can you wrap it in using? #Resolved
| 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(); |
There was a problem hiding this comment.
Done(); [](start = 12, length = 7)
You don't have to call Done if you don't do any basefiles comparison #Resolved
| Assert.True(n == 2); | ||
| } | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'll do it. #Resolved
xadupre
changed the title
|
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
changed the title
dotnet#587) `DataViewConstructionUtils`'s methods to create dataviews over .NET types will now have correctly inferred "getters" in the case of sparse vectors.
ghost
locked as resolved and limited conversation to collaborators
Fixes #586.
DataViewConstructionUtils's methods to create dataviews over .NET types will now have correctly inferred getters in the case of sparse vectors.