Skip to content
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

Fix wrong type conversion on PrimitiveDataFrameColumn #6834

Merged
merged 6 commits into from
Sep 25, 2023

Conversation

novelhawk
Copy link
Contributor

@novelhawk novelhawk commented Sep 20, 2023

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
Copy link

codecov bot commented Sep 20, 2023

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%) ⬆️
production 63.59% <100.00%> (+0.03%) ⬆️
test 88.86% <100.00%> (+<0.01%) ⬆️

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
Copy link
Contributor

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

@novelhawk
Copy link
Contributor Author

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
Copy link
Contributor

@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:

novelhawk#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.

Add files generated from tt scripts and fix unit tests
@novelhawk
Copy link
Contributor Author

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

@LittleLittleCloud LittleLittleCloud merged commit 49824f3 into dotnet:main Sep 25, 2023
23 checks passed
JakeRadMSFT added a commit that referenced this pull request Oct 5, 2023
* Update build templates to handle feature branches (#6744)

* Update build templates

* Update build templates to include all releases/* and feature/*

* Update releases to release

* Update triggers for PR Validation Build

* Add triggers for Code Coverage

* Update version to 4.0 for feature branch (#6743)

* Add missing implementation for datetime relevant arrow type into dataframe (#6675)

* Add missing implementation for datetime relevant arrow type

* Return required usage

* Fix the behavior or column SetName method (#6676)

* Fix the behavior or column SetName method

* Fix stack overflow exception

* Fix merge issues

---------

Co-authored-by: Michael Sharp <51342856+michaelgsharp@users.noreply.github.com>

* Fix DataFrame to allow to store columns with size more than 2 Gb (#6710)

* Fix error with allocating more than MaxCapacity of Byte Memory Buffer

* Remove Unit test as it consumes too much memory

* Fix issue with increasing buffer capacity over limit when double it size

* avoid empty dataset (#6756)

* Fix dataframe arithmetics for columns having several value buffers (column size is more than 2 Gb) (#6724)

* Fix dataframe arithmetics

* Fix

* Run tests that requires more than 2 Gb of Memory only on 64-bit env (#6758)

* Reduce coupling of Data.Analysis.Tests project (#6759)

* Provide ability to filter dataframe column by null via ElementWise Methods (#6723)

* Provide ability to filter by null value

* Add comments

* Fix code review findings

* Fix incorrect DataFrame min max computation with NULL (#6734)

* Step 1

* Step 2

* Fixed code review findings

* Clean DataFrame meaningless code (#6761)

* Add NameEntityRecognition and Q&A deep learning tasks. (#6760)

* NER

* QA almost done, runtime error

* QA finished

* fixes from PR comments

* fixed build

* build fixes

* perf changes

* made disposable

* fixed not disposing model

* added some disposables to TensorFlow for memory

* build testing

* fixing build

* added missing dispose

* build fixes

* build fixes

* testing macos fix

* fix issue (#6768)

* fixed mac build and minor torch sharp changes (#6776)

* Improve DataFrame Arithmetics implementation (#6763)

* Change methods signature generation

* Change DataFrameColumn Arithmetics

* Change DataFrameColumn Operations

* Fix unit tests

* Fix spaces

* Fix code review findings

* Add QA sweepable estimator in AutoML  (#6781)

* Add QA sweepable

* clean

* Modernized some argument checks that still used string literals for parameter names (#6766)

Co-authored-by: John Doe <john@doe>

* removed deprecated yosemite brew (#6805)

* Add TargetType to Type_convert (#6785)

* Add target Type in convert  type

* Add custom type "DataKind"

* clean

* Add DataKind name space

* clean test

* File-scoped namespaces in files under `Environment` (`Microsoft.ML.Core`) (#6791)

Co-authored-by: Lehonti Ramos <john@doe>

* File-scoped namespaces in files under `EntryPoints` (`Microsoft.ML.Core`) (#6790)

Co-authored-by: Lehonti Ramos <john@doe>

* Fix issue with addIndexColumn in DataFrame.LoadCsv (#6769)

* Fix issue with addIndexColumn in DataFrame.LoadCsv

* Fix tests

* Fix DataFrame.LoadCsv can not load CSV with duplicate column names (#6772)

* File-scoped namespaces in files under `ComponentModel` (`Microsoft.ML.Core`) (#6788)

Co-authored-by: Lehonti Ramos <john@doe>

* File-scoped namespaces in files under `Data` (`Microsoft.ML.Core`) (#6789)

Co-authored-by: Lehonti Ramos <john@doe>

* Fix inconsistent null handling in DataFrame Arithmetics (#6770)

* Fix inconsistent null handling in DataFrame Arithmetics

* Fix Null Count and division by zero issues

* Minor changes to restart build and rerun flaky tests

* File-scoped namespaces in files under `Prediction` (`Microsoft.ML.Core`) (#6792)

Co-authored-by: Lehonti Ramos <john@doe>

* Allow to define CultureInfo for parsing values on reading DataFrame from csv (#6782)

* Use CultureInfo for parsing values in csv file

* Fix merge issues

* Append dataframe rows based on column names (#6808)

* Append dataframe rows based on column names

* Update DataFrame.cs

---------

Co-authored-by: Michael Sharp <51342856+michaelgsharp@users.noreply.github.com>

* removed codecov token (#6811)

* Fix wrong type conversion on PrimitiveDataFrameColumn (#6834)

* Fix wrong type conversion on PrimitiveDataFrameColumn

* Added tests for #6829

* Fix test

* Add file generated from tt template and fix unit tests

---------

Co-authored-by: Aleksei Smirnov <tlalok@inbox.ru>

* update interactive kernel version (#6836)

* update interactive kernel version

* update

* Update Microsoft.Data.Analysis.Interactive.Tests.csproj

* Add performance benchmarks for dataframe arithmetic operations  (#6827)

* Add performance tests

* Add extra tests

* Fix

* Fix typo

* Fix Divide_Int16 and Divide_Int32_Int16 benchmarks

* Fix

* Change csproj file

* Update BenchmarkDotNetVersion to 0.13.5

* Fix

* Change to 0.13.1 because that is what is latest version in our nuget feeds.

---------

Co-authored-by: Jake Radzikowski <JakeRad@Microsoft.com>

* Improve performance of column cloning inside DataFrame arithmetics (#6814)

* Optimize PrimitiveColumnContainer.Clone method

* Avoid unnecessary type conversion during binary operations

* Remove using

* Fix DataFrameBuffer constructor

* remove uncorrectly added using

* Make DataFrameBuffer Length field protected

* Fix typo

* Use RawSpan

* Simplify tt files for PrimitiveDataFrameColumnAritmetics (#6830)

* First step of tt refactoring

* Step 2

* Step 3

* Addresses #6533 (#6838)

* Initial structure and started fleshing out some sections

* Some corrections and paragraph on DL usages

* Starting fleshing out DL on ML.NET section

* Addresses #6533

* Update dependencies (#6837)

* Update dependencies

* Add reference to NuGet.Packaging.Core

* PrimitiveDataFrameColumn.Clone method crashes when is used with IEnumerable mapIndices argument (#6822)

* Split Test for AppendMany into 4 different tests

* Block init of null validity buffer instead of setting individual bits

* Add unit tests for PrimitiveDataFrameColumn.Clone

* Fixes #6821

* Fix

* Fix bug with AppendMany values to not empty column

* Restart unit tests

* Add more unit tests

* Fix failing unit test

* Fix code review findings

* 6847 incorrectly sets column value (#6849)

* Fix DataFrame incorrectly sets column value for index higher than Buffer.MaxCapacity

* Revert renaming

* Increase performance of arithmetic operations by enhancing calculations on nullable values  (#6846)

* Optimize PrimitiveColumnContainer.Clone method

* Avoid unnecessary type conversion during binary operations

* Remove using

* Fix DataFrameBuffer constructor

* remove uncorrectly added using

* Make DataFrameBuffer Length field protected

* Add performance tests

* Split Test for AppendMany into 4 different tests

* Block init of null validity buffer instead of setting individual bits

* Add unit tests for PrimitiveDataFrameColumn.Clone

* Fixes #6821

* Fix

* Add extra tests

* Fix

* Fix typo

* Fix Divide_Int16 and Divide_Int32_Int16 benchmarks

* Fix

* Avoid using constructor, that copies memory

* First step of tt refactoring

* Step 2

* Step 3

* Move iteration over buffers outside of the PrimitiveDataFrameColumnArithmetic

* Change PrimitiveDataFrameColumnArithmetic

* Fix typo

* Use RawSpan

* Fix bug with AppendMany values to not empty column

* Restart unit tests

* Add more unit tests

* Add GetBitCount method

* Fix failing unit test

* Implementation

* Change unit tests

* Update unit tests

* Refactoring BinaryOperation

* Intermediate changes

* Intermediate results

* Implement Binary Scalar Reverse Operarions

* Add implementation for BinaryIntOperations

* Implement Comparison Operations

* Implement actual calculations for Comparison operations

* Uncomment performance tests

* Remove unintentional code changes

* Add reference to Apache Arrow project license in THIRD-PARTY-NOTICES

* Fix license issues

* Fixes incorrect work of DataFrame with VBufferColumn when number of e… (#6851)

* Fixes incorrect work of DataFrame with VBufferColumn when number of elements is greater than Int.MaxValue

* Fix calculation of max capacity and amount of required buffers

* Fix unit test

* Run test allocating more than 2 Gb of memory on 64bit env only

* Fix StringDataFrameColumn same way as VBufferDataFrameColumn

* Fix wrong amount of buffers created in constructor of StringDataFrameColumn

* Fix code review findings

---------

Co-authored-by: Aleksei Smirnov <tlalok@inbox.ru>
Co-authored-by: Michael Sharp <51342856+michaelgsharp@users.noreply.github.com>
Co-authored-by: Xiaoyun Zhang <xiaoyuz@microsoft.com>
Co-authored-by: zewditu Hailemariam <36615490+zewditu@users.noreply.github.com>
Co-authored-by: Lehonti Ramos <17771375+Lehonti@users.noreply.github.com>
Co-authored-by: John Doe <john@doe>
Co-authored-by: Raffaello Fraboni <10281615+novelhawk@users.noreply.github.com>
Co-authored-by: R. G. Esteves <rodolfo.g.esteves@intel.com>
Co-authored-by: Eric StJohn <ericstj@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataFrame] Null Reference on ElementwiseEquals on DateTime columns from DataFrame
3 participants