Skip to content

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

Merged
harishsk merged 5 commits intodotnet:masterfrom
yeeyang19:yeeyang/equal-issue-fix
Aug 25, 2020
Merged

Fixes #5352 issues caused by equality with non-string values for root cause localization #5354
harishsk merged 5 commits intodotnet:masterfrom
yeeyang19:yeeyang/equal-issue-fix

Conversation

@yeeyang19
Copy link
Copy Markdown
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
Copy Markdown

dnfadmin commented Aug 19, 2020

CLA assistant check
All CLA requirements met.

Comment thread src/Microsoft.ML.TimeSeries/RootCauseAnalyzer.cs Outdated
Comment thread src/Microsoft.ML.TimeSeries/RootCauseAnalyzer.cs Outdated
Comment thread src/Microsoft.ML.TimeSeries/RootCauseAnalyzer.cs
Comment thread src/Microsoft.ML.TimeSeries/RootCauseAnalyzer.cs Outdated
Comment thread src/Microsoft.ML.TimeSeries/RootCauseAnalyzer.cs Outdated
@codecov
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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.

Comment thread test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs
Comment thread test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs Outdated
Comment thread test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs
Comment thread test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs
Comment thread test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs Outdated
Comment thread test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs Outdated
Comment thread test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs Outdated
@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

Development

Successfully merging this pull request may close these issues.

LocalizeRootCause does not return any root causes when RootCauseLocalizationInput includes dimensions with values of type long

6 participants