added accessors.js, changed related files and added tests #5971
Conversation
stdlib-bot
added
Statistics
Coverage Report
The above coverage report was generated for the changes in this PR. |
|
@kgryte please let me know if further changes needed |
| var i; | ||
|
|
||
| // Check for accessor array: | ||
| if ( x.get ) { |
There was a problem hiding this comment.
This isn't a sufficient check. Please follow what we do in other implementations supporting the accessor protocol.
| ix = offset; | ||
|
|
||
| // Use an estimate for the mean: | ||
| mu = x.get( ix ); |
There was a problem hiding this comment.
This is not what we want. Follow the conventions in other packages already having accessor protocol support.
|
|
||
| // MODULES // | ||
|
|
||
| var ndarray = require( './ndarray.js' ); |
There was a problem hiding this comment.
Please study https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/base/max and emulate accordingly.
| var v; | ||
|
|
||
| x = { | ||
| 'get': function get( i ) { |
There was a problem hiding this comment.
This is not what we want. Use toAccessorArray as in other implementations supporting accessor arrays. In short, emulate as closely as possible what we do in other packages.
There was a problem hiding this comment.
Left an initial round of comments. In short, you need to more closely emulate what we do in https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/base/max
kgryte
added
Needs Changes
3 participants
Resolves #5688 .
Description
This pull request:
It passes all tests.
Related Issues
This pull request:
stats/base/variancech#5688Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers