Skip to content

Conversation

Krishna-Sharma-g
Copy link
Contributor

@Krishna-Sharma-g Krishna-Sharma-g commented Mar 3, 2025

Resolves #{{TODO: add issue number}}.

#5626

What is the purpose of this pull request?

This pull request:

added the stats/incr/nanvariance 

Related Issues

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

all three test commands passes

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@stdlib-bot stdlib-bot added Statistics Issue or pull request related to statistical functionality. Needs Review A pull request which needs code review. Good First PR A pull request resolving a Good First Issue. labels Mar 3, 2025
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Mar 3, 2025

Coverage Report

Package Statements Branches Functions Lines
stats/incr/nanvariance $\color{green}174/174$
$\color{green}+100.00\%$
$\color{green}10/10$
$\color{green}+100.00\%$
$\color{green}2/2$
$\color{green}+100.00\%$
$\color{green}174/174$
$\color{green}+100.00\%$

The above coverage report was generated for the changes in this PR.

Signed-off-by: Gautam sharma <[email protected]>
Signed-off-by: Gautam sharma <[email protected]>
Signed-off-by: Gautam sharma <[email protected]>
@Krishna-Sharma-g Krishna-Sharma-g changed the title [RFC]: add stats/incr/nanvariance #5626 feat: add stats/incr/nanvariance function Mar 5, 2025
@Krishna-Sharma-g
Copy link
Contributor Author

@kgryte please review once it has some linting issues that i will fix soon but please review the main implementation of it and suggest the changes i can do in it

@Krishna-Sharma-g
Copy link
Contributor Author

hello @anandkaranubc please review

@@ -0,0 +1,57 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

This entire file is incorrect. Please follow our repl text style guide and avoid using LLMs to generate files.

Copy link
Contributor Author

@Krishna-Sharma-g Krishna-Sharma-g Mar 12, 2025

Choose a reason for hiding this comment

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

it was showing warning in the linting years so i added that licence code in that but now i have resolved it please check is that exactly you wanted


// The compiler throws an error if the function is provided invalid arguments...
{
incrnanvariance( '5' as any ); // $ExpectError
Copy link
Member

Choose a reason for hiding this comment

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

Remove all of these as any casts. The entire point of these lines is to test that the compiler errors when provided various non-any input values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i did it because it was showing some ecmacript error in it but i have changed it now and removed the as any from there please review

* v = accumulator();
* // returns 33.7796
*/
function incrnanvariance( mean ) {
Copy link
Member

Choose a reason for hiding this comment

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

This implementation is not what is desired. As mentioned in the OP,

In particular notice how nansum is a thin wrapper around sum. In most cases, this is what we are looking for.

You're essentially duplicating the incr/variance implementation. That is not what we want. Create a thin wrapper similar to nansum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i got confused between both i have changed please check

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

Left an initial round of comments. This PR needs a fair amount of refactoring and clean-up before it can be merged.

@kgryte kgryte added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Mar 12, 2025
@Krishna-Sharma-g
Copy link
Contributor Author

okay @kgryte working on it

@Krishna-Sharma-g
Copy link
Contributor Author

Krishna-Sharma-g commented Mar 12, 2025

@kgryte i did the changes in this you wanted me to change and i have fixed the linting issues also please review and i am creating the readme for nanvariance also

@Krishna-Sharma-g
Copy link
Contributor Author

@kgryte please review !!

@Krishna-Sharma-g
Copy link
Contributor Author

@kgryte please review !

@kgryte
Copy link
Member

kgryte commented Apr 2, 2025

@Krishna-Sharma-g This package is missing a README file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Good First PR A pull request resolving a Good First Issue. Needs Changes Pull request which needs changes before being merged. Statistics Issue or pull request related to statistical functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC]: add stats/incr/nanvariance

3 participants