First pass at adding std deviation to measure-object #4969

dfinke · 2017-10-01T17:16:36Z

SteveL-MSFT · 2017-10-01T18:01:57Z

Added the issue

iSazonov · 2017-10-02T05:52:44Z

@dfinke Thanks for your contribution!

The tests file contains multiple formatting issue. We should split code and formatting changes. So I open #4972 - please rebase after it will be merged.

We should think about optimizations. Currently we do multiple checks _measureSum, _measureAverage, _measureStdDeviation - can we refactor and optimize?

SteveL-MSFT · 2017-10-02T19:28:55Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

+
+        /// <summary>
+        ///
+        /// The StdDeviation of property values


For the comments, we should spell out Standard Deviation

SteveL-MSFT · 2017-10-02T19:29:31Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

+
+        /// <summary>
+        ///
+        /// The StdDeviation of property values


For the comments, we should spell out Standard Deviation

SteveL-MSFT · 2017-10-02T19:32:39Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

+                return _measureStdDeviation;
+            }
+            set
+            {


stddeviation requires _measureAverage, so I think we should set _measureAverage = true. If _measureAverage = false, perhaps we should error.

Makes sense. Where should it throw, in CreateGenericMeasureInfo?

Yes, I think that's the right place.

I wonder why we request Average parameter if we can set _measureAverage = true here?

Thinking about this, it seems that -Average:$false -StdDeviation should not be a valid combination and perhaps a better way to solve this is to make them different parameter sets. The problem here is that depending on which one is set first, you may get unintended behavior -StdDeviation -Average:$false since the code here overwrites _measureAverage.

We need new flag to process a sum. We can set it in BeginProcessing.

SteveL-MSFT · 2017-10-02T19:36:28Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

+
+                    foreach (double n in stat.stdDeviationNumbers)
+                    {
+                        popdev += Math.Pow((n - (double)average), 2);


My understanding is that A*A is faster than Math.Pow(A,2)

SteveL-MSFT · 2017-10-02T19:38:45Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

+
+                if (_measureStdDeviation && _measureAverage && stat.count > 0)
+                {
+                    var popdev = 0.0;


I think sumOfDerivation is more descriptive than popdev

dfinke · 2017-10-02T20:33:33Z

I noticed that the property StdDeviation doesn't show up in help measure. Do the maml files need to get updated and rebuilt? I tweaked one of the maml files, but no luck.

…d comments

SteveL-MSFT · 2017-10-02T22:14:58Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

                    average = stat.sum / stat.count;
+
+                if(_measureStdDeviation && !_measureAverage) {
+                    var message = "StdDeviation was requested and requires the average to be calculated, please add the -Average switch";


messages should be in the corresponding .resx file. See line 537 as an example.

SteveL-MSFT · 2017-10-02T22:15:58Z

@dfinke the MAML files are generated. You need to update the help found in https://github.com/powershell/powershell-docs

SteveL-MSFT · 2017-10-03T00:57:04Z

src/Microsoft.PowerShell.Commands.Utility/resources/MeasureObjectStrings.resx

    <value>Input object "{0}" is not numeric.</value>
  </data>
+  <data name="AverageSwitchNotSet" xml:space="preserve">
+    <value>StdDeviation was requested and requires the average to be calculated, please add the -Average switch.</value>


I think we should default -Average to be set if -StdDeviation is set, I think the error only applies if someone explicitly used -Average:$false. So I think the error message should be a bit more specific:

StdDeviation was requested and requires the average to be calculated, however '-Average' was set to $false.

iSazonov · 2017-10-03T03:41:19Z

@dfinke #4972 was merged. Please resolve merge conflict.

iSazonov · 2017-10-03T03:53:31Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

+                return _measureStdDeviation;
+            }
+            set
+            {


I wonder why we request Average parameter if we can set _measureAverage = true here?

iSazonov · 2017-10-03T03:57:12Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

                if (_measureAverage && stat.count > 0)
                    average = stat.sum / stat.count;
+
+                if(_measureStdDeviation && !_measureAverage) {


I think it must be Diagnostic.Assert().

With different parameter sets, this code is not needed.

iSazonov · 2017-10-03T04:08:42Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

+
+                    foreach (double n in stat.stdDeviationNumbers)
+                    {
+                        var m = n - (double)average;


Should we move the cast out of foreach ?

iSazonov · 2017-10-03T04:15:06Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

+                        sumOfDerivation += m * m;
+                    }
+
+                    stdDeviation = Math.Round(Math.Sqrt(sumOfDerivation / (stat.stdDeviationNumbers.Count - 1)), 4);


Please clarify why we round? I believe it must be a double.

I rounded for no particular reason. I can remove it.

lzybkr · 2017-10-03T04:48:37Z

Maybe not for this PR - but the output of Measure-Object is not great - most of the properties are $null, and adding a new property will make it slightly worse.

I was going to propose something like what I use in my profile (see here), but that is messy and not quite right (doesn't support strict mode properly), so I might actually want a new formatting feature to make it simpler to specify optional properties, something like:

<ListItem>
    <PropertyName WhenPresent="true">Average</PropertyName>
</ListItem>

This could test if the property is present and non-null before selecting the item.

iSazonov · 2017-10-03T04:55:06Z

Can we create a custom PSObject with non-null properties in the cmdlet?

SteveL-MSFT · 2017-10-03T19:45:52Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

+                return _measureStdDeviation;
+            }
+            set
+            {


Thinking about this, it seems that -Average:$false -StdDeviation should not be a valid combination and perhaps a better way to solve this is to make them different parameter sets. The problem here is that depending on which one is set first, you may get unintended behavior -StdDeviation -Average:$false since the code here overwrites _measureAverage.

SteveL-MSFT · 2017-10-03T19:46:14Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Measure-Object.cs

                if (_measureAverage && stat.count > 0)
                    average = stat.sum / stat.count;
+
+                if(_measureStdDeviation && !_measureAverage) {


With different parameter sets, this code is not needed.

dfinke · 2017-10-03T19:59:13Z

Another option might be to calculate average when std deviation is specified. Or use the already calculated average if the -Average switch was passed.

dfinke · 2017-10-05T15:05:32Z

I'm confused. I checked the type etc. In the .cs file I took out the Math.Round.

iSazonov · 2017-10-07T07:39:29Z

@dfinke Please remove Math.Round because we should return honest double - the round is only formatting. We have separate formatting engine as @lzybkr mentioned above. If we want better formatting we should make it there. I believe we can skip this in the PR.

Please resolve merge conflict and address feedbacks.

iSazonov · 2017-10-30T09:43:06Z

@dfinke Could you please continue?

dfinke · 2017-10-30T13:49:18Z

@iSazonov sorry, my free time got pulled in a different direction. I also got blocked testing. Returning the double from the cmdlet and testing it in Pester. The number looked correct from the cmdlet and in Pester to several decimal places.

I won't be able to get back to this in the near future.

iSazonov · 2017-10-31T09:41:05Z

@dfinke Very sorry. Welcome back with new ideas and contributions!

First pass at adding std deviation to measure-object

54a29cd

dfinke requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners October 1, 2017 17:16

SteveL-MSFT added the Hacktoberfest Potential candidate to participate in Hacktoberfest label Oct 1, 2017

iSazonov mentioned this pull request Oct 2, 2017

Reformat Measure-Object test #4972

Merged

SteveL-MSFT requested changes Oct 2, 2017

View reviewed changes

Use A*A instead of Math.Pow. Throw if -Average not specified and fixe…

06dfb2f

…d comments

SteveL-MSFT reviewed Oct 2, 2017

View reviewed changes

Added message to the rex file

eb541c5

SteveL-MSFT requested changes Oct 3, 2017

View reviewed changes

iSazonov reviewed Oct 3, 2017

View reviewed changes

dfinke added 3 commits October 3, 2017 13:53

Set Average true if StdDeviation is set

ca4b539

Update message

806f3a7

Update tests

c95b829

SteveL-MSFT requested changes Oct 3, 2017

View reviewed changes

daxian-dbw assigned iSazonov Oct 6, 2017

iSazonov closed this Oct 31, 2017

CloudyDino mentioned this pull request Feb 24, 2018

Standard deviation implementation on Measure-Object #6238

Merged

11 tasks

First pass at adding std deviation to measure-object #4969

First pass at adding std deviation to measure-object #4969

Uh oh!

Conversation

dfinke commented Oct 1, 2017 • edited by SteveL-MSFT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Oct 1, 2017

Uh oh!

iSazonov commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfinke commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr commented Oct 3, 2017

Uh oh!

iSazonov commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfinke commented Oct 3, 2017

Uh oh!

dfinke commented Oct 5, 2017

Uh oh!

iSazonov commented Oct 7, 2017

Uh oh!

iSazonov commented Oct 30, 2017

Uh oh!

dfinke commented Oct 30, 2017

Uh oh!

iSazonov commented Oct 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

dfinke commented Oct 1, 2017 •

edited by SteveL-MSFT

Loading

iSazonov commented Oct 3, 2017 •

edited

Loading