-
-
Notifications
You must be signed in to change notification settings - Fork 968
feat: add stats/base/ndarray/dmeanstdev
#8525
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
base: develop
Are you sure you want to change the base?
feat: add stats/base/ndarray/dmeanstdev
#8525
Conversation
Coverage Report
The above coverage report was generated for the changes in this PR. |
|
|
||
| <section class="intro"> | ||
|
|
||
| The [arithmetic mean][arithmetic-mean] and [standard deviation][standard-deviation] are defined as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should just copy the introduction from stats/strided/dmeanstdev instead of rolling your own. This makes find-and-replace easier when we are consistent in our docs across similar packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment still needs to be addressed.
| function dmeanstdev( arrays ) { | ||
| var out = arrays[ 1 ]; | ||
| var x = arrays[ 0 ]; | ||
| return strided( numelDimension( x, 0 ), 1, getData( x ), getStride( x, 0 ), getOffset( x ), getData( out ), getStride( out, 0 ), getOffset( out ) ); // eslint-disable-line max-len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what is desired. For a much more complicated API, see https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/base/ndarray/dztest. Notice there that scalar parameters, such as should be done for correction, are provided as ancillary arrays. Hardcoding 1 for the correction is not what we want.
lib/node_modules/@stdlib/stats/base/ndarray/dmeanstdev/README.md
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/ndarray/dmeanstdev/README.md
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/stats/base/ndarray/dmeanstdev/README.md
Outdated
Show resolved
Hide resolved
kgryte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some initial comments.
Co-authored-by: Athan <[email protected]> Signed-off-by: Sachin Pangal <[email protected]>
Co-authored-by: Athan <[email protected]> Signed-off-by: Sachin Pangal <[email protected]>
Co-authored-by: Athan <[email protected]> Signed-off-by: Sachin Pangal <[email protected]>
|
@kgryte Done — I’ve applied all the requested changes. |
| * | ||
| * @param arrays - array-like object containing input and output ndarrays | ||
| * @returns output ndarray containing [ mean, stdev ] | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing an example.
|
|
||
| /* eslint-disable space-in-parens */ | ||
|
|
||
| import zeros = require( '@stdlib/ndarray/zeros' ); // eslint-disable-line import/no-unresolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import zeros = require( '@stdlib/ndarray/zeros' ); // eslint-disable-line import/no-unresolved | |
| import zeros = require( '@stdlib/ndarray/zeros' ); |
This and similar comments should be removed. They are not applicable.
| 'dtype': 'float64' | ||
| }); | ||
|
|
||
| dmeanstdev( [ x, out, correction ] ); // $ExpectType ndarray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| dmeanstdev( [ x, out, correction ] ); // $ExpectType ndarray | |
| dmeanstdev( [ x, out, correction ] ); // $ExpectType float64ndarray |
| * @param arrays - array-like object containing input and output ndarrays | ||
| * @returns output ndarray containing [ mean, stdev ] | ||
| */ | ||
| declare function dmeanstdev( arrays: [ float64ndarray, float64ndarray ] ): float64ndarray; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing the correction parameter ndarray.
| dmeanstdev( [ x, out ], {} ); // $ExpectError | ||
| dmeanstdev( [ x, out ] ); // $ExpectError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not providing the correction parameter in the list of arrays.
| * var correction = scalar2ndarray( 1.0, opts ); | ||
| * | ||
| * var v = dmeanstdev( [ x, out, correction ] ); | ||
| * // returns <ndarray> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should actually display the expected results (i.e., the mean and stdev) using ndarray2array.
| * var correction = scalar2ndarray( 1.0, opts ); | ||
| * | ||
| * var v = dmeanstdev( [ x, out, correction ] ); | ||
| * // returns <ndarray> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment.
kgryte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left another round of comments.
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes. report:
Resolves None.
Description
This pull request:
Related Issues
This pull request has the following related issues:
Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
{{TODO: add disclosure if applicable}}
@stdlib-js/reviewers