Skip to content

Fixes #5352 issues caused by equality with non-string values for root cause localization #5354

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

Merged
merged 5 commits into from
Aug 25, 2020

Conversation

yeeyang19
Copy link
Contributor

Fixes #5352

Comparison between dimension value and aggregated symbol failed or got the incorrect result when their values weren't string.
Need to support nullable object as the dimension value and aggregated symbol, so we use Object.Equals(a, b) instead of a == b and a.Equals(b).

Besides, I replace object using Object to align with the data type of dimension and aggregated symbol in this extension.

@yeeyang19 yeeyang19 requested a review from a team as a code owner August 19, 2020 11:43
@dnfadmin
Copy link

dnfadmin commented Aug 19, 2020

CLA assistant check
All CLA requirements met.

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #5354 into master will decrease coverage by 2.59%.
The diff coverage is 98.00%.

@@            Coverage Diff             @@
##           master    #5354      +/-   ##
==========================================
- Coverage   74.04%   71.45%   -2.60%     
==========================================
  Files        1019     1019              
  Lines      190035   190163     +128     
  Branches    20457    20459       +2     
==========================================
- Hits       140720   135881    -4839     
- Misses      43783    48831    +5048     
+ Partials     5532     5451      -81     
Flag Coverage Δ
#Debug 71.45% <98.00%> (-2.60%) ⬇️
#production 67.41% <50.00%> (-2.44%) ⬇️
#test 84.50% <100.00%> (-3.15%) ⬇️

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

Impacted Files Coverage Δ
...crosoft.ML.TimeSeries/RootCauseLocalizationType.cs 48.31% <0.00%> (ø)
src/Microsoft.ML.TimeSeries/RootCauseAnalyzer.cs 57.36% <75.00%> (+2.73%) ⬆️
...crosoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs 99.70% <100.00%> (+0.06%) ⬆️
test/Microsoft.ML.Predictor.Tests/TestIniModels.cs 0.00% <0.00%> (-100.00%) ⬇️
.../Microsoft.ML.Ensemble/OutputCombiners/Stacking.cs 0.00% <0.00%> (-100.00%) ⬇️
...osoft.ML.Ensemble/OutputCombiners/MultiStacking.cs 0.00% <0.00%> (-100.00%) ⬇️
...soft.ML.Predictor.Tests/TestGamPublicInterfaces.cs 0.00% <0.00%> (-100.00%) ⬇️
...ft.ML.Core/CommandLine/DefaultArgumentAttribute.cs 0.00% <0.00%> (-100.00%) ⬇️
...t.ML.Core/CommandLine/EnumValueDisplayAttribute.cs 0.00% <0.00%> (-100.00%) ⬇️
....ML.Ensemble/OutputCombiners/BaseScalarStacking.cs 0.00% <0.00%> (-100.00%) ⬇️
... and 122 more

Copy link
Contributor

@frank-dong-ms-zz frank-dong-ms-zz left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@lisahua lisahua 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 adding tests. LEave some styling suggestions.

@harishsk harishsk merged commit 3fe6cdf into dotnet:master Aug 25, 2020
@yeeyang19 yeeyang19 deleted the yeeyang/equal-issue-fix branch August 26, 2020 02:05
@ghost ghost locked as resolved and limited conversation to collaborators Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants