dotnet / machinelearning

ML.NET is an open source and cross-platform machine learning framework for .NET.
https://dot.net/ml
MIT License
9.02k stars 1.88k forks source link

Fix wrong type conversion on PrimitiveDataFrameColumn #6834

Closed novelhawk closed 1 year ago

novelhawk commented 1 year ago

Fixes #6829

Adds the patch commented by @asmirnov82 in #6831

Updated only the .tt template, tried generating the .cs through Visual Studio but it leaves blank lines where the templating is (the current files don't have it so I immagine there is a next step/I'm doing something wrong)

Also couldn't manage to run tests due to errors with arcade and formatting during compilation.

codecov[bot] commented 1 year ago

Codecov Report

Merging #6834 (7c07dc9) into main (d692751) will increase coverage by 0.02%. Report is 1 commits behind head on main. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #6834      +/-   ##
==========================================
+ Coverage   68.99%   69.02%   +0.02%     
==========================================
  Files        1237     1237              
  Lines      253558   253564       +6     
  Branches    26542    26542              
==========================================
+ Hits       174944   175024      +80     
+ Misses      71663    71591      -72     
+ Partials     6951     6949       -2     
Flag Coverage Δ
Debug 69.02% <100.00%> (+0.02%) :arrow_up:
production 63.59% <100.00%> (+0.03%) :arrow_up:
test 88.86% <100.00%> (+<0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage
...lysis/PrimitiveDataFrameColumn.BinaryOperations.cs 100.00%
...st/Microsoft.Data.Analysis.Tests/DataFrameTests.cs 100.00%
LittleLittleCloud commented 1 year ago

Did you include the generated .cs file from .tt as well. Can't find it in this PR.

novelhawk commented 1 year ago

No I had problems generating them.. The generated files were full of blank lines that the ones in the repo don't have, what's the correct way to generate them? Also couldn't run test due to formatting compilation errors but they seem to pass in CI

asmirnov82 commented 1 year ago

@novelhawk, you can generate tt files using text transform utility in command line (https://learn.microsoft.com/en-us/visualstudio/modeling/generating-files-with-the-texttransform-utility) or just saving changed template in VS. I opened your branch and generated file on my laptop - it's fine, no extra spaces. I included it into PR to your repo:

https://github.com/novelhawk/machinelearning/pull/1

I also update you unit tests (currently it's running successfuly even on branch without actual fix). Fix is related to the issue of calling ElementwiseEquals with scalar DateTime parameter, so I change the test to use SampleData as paramer df["DateTime1"].ElementwiseEquals(SampleDateTime); only first element in the DateTime1 column is equal to it, so only equalsToScalarResult[0] should be True. You don't have to reopened this PR, it should be updated automaticaly after fix is merged to your repo.

novelhawk commented 1 year ago

I'm probably missing some dependencies or maybe I have an outdated version of the generator, running TextTransformCore --version yields 17.6.36396+561222c835 and I have installed VS 2022 Community if this is investigated in the future.

Anyhow thanks for the help, I've merged your PR