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

Use Equals and = operator for DataViewType comparison #5942

Merged
merged 1 commit into from
Oct 1, 2021
Merged

Use Equals and = operator for DataViewType comparison #5942

merged 1 commit into from
Oct 1, 2021

Conversation

thoron
Copy link
Contributor

@thoron thoron commented Sep 23, 2021

This PR expands on the DataViewType equals comparison to allow for advanced types to be used. Currently columns of type Vector<T, NN> does to work as the comparison will fail. The Equals implementation in VectorDataViewType does account for the PrimitiveDataViewType and dimension sizes.

The method chosen simply expands on the current comparsion and adds Equals to it.

The code has not been tested using the existing test suits as I could not get them to run locally.

This PR also contains two style fixes, shown as errors in VS (automatically fixed).

Fixes #5773

DataViewType of Vector<T, NN> would not evaluate correctly and Vector types which was of same types and sizes would fail for AutoML.

Fixes #5773
Copy link
Member

@michaelgsharp michaelgsharp left a comment

Choose a reason for hiding this comment

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

Thanks for making this update. LGTM.

We had an issue with the code coverage not uploading correctly. It has been fixed, so I'll see if a fresh run of the pipelines will get the change. If not, you will need to rebase on main.

@michaelgsharp
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@codecov
Copy link

codecov bot commented Oct 1, 2021

Codecov Report

Merging #5942 (f55129d) into main (3bf8cba) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5942      +/-   ##
==========================================
+ Coverage   68.19%   68.20%   +0.01%     
==========================================
  Files        1142     1142              
  Lines      242367   242534     +167     
  Branches    25355    25378      +23     
==========================================
+ Hits       165282   165430     +148     
- Misses      70406    70407       +1     
- Partials     6679     6697      +18     
Flag Coverage Δ
Debug 68.20% <100.00%> (+0.01%) ⬆️
production 62.87% <100.00%> (-0.01%) ⬇️
test 88.59% <ø> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...crosoft.ML.AutoML/Utils/UserInputValidationUtil.cs 91.45% <100.00%> (ø)
....ML.AutoML/PipelineSuggesters/PipelineSuggester.cs 81.88% <0.00%> (-3.15%) ⬇️
src/Microsoft.ML.Sweeper/AsyncSweeper.cs 71.42% <0.00%> (-1.37%) ⬇️
...soft.ML.Transforms/Text/WordEmbeddingsExtractor.cs 85.74% <0.00%> (-1.14%) ⬇️
src/Microsoft.Data.Analysis/Strings.Designer.cs 43.26% <0.00%> (-0.95%) ⬇️
src/Microsoft.ML.AutoML/Sweepers/Parameters.cs 84.74% <0.00%> (-0.85%) ⬇️
...rosoft.Data.Analysis/ArrowStringDataFrameColumn.cs 63.54% <0.00%> (-0.47%) ⬇️
...StandardTrainers/Standard/LinearModelParameters.cs 65.82% <0.00%> (-0.26%) ⬇️
src/Microsoft.Data.Analysis/DataFrame.Join.cs 94.50% <0.00%> (-0.23%) ⬇️
...st/Microsoft.Data.Analysis.Tests/DataFrameTests.cs 99.21% <0.00%> (-0.22%) ⬇️
... and 5 more

@michaelgsharp michaelgsharp merged commit d4c9dd4 into dotnet:main Oct 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Vector<Type, NN> does not work as AutoML validation data
2 participants