Skip to content

Conversation

@MatthewCochrane
Copy link
Contributor

Resolves #45 .

Checklist

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

  • Read, understood, and followed the contributing guidelines, including the relevant style guides.
  • Read and understand the Code of Conduct.
  • Read and understood the licensing terms.
  • Searched for existing issues and pull requests before submitting this pull request.
  • Filed an issue (or an issue already existed) prior to submitting this pull request.
  • Rebased onto latest develop.
  • Submitted against develop branch.

Description

What is the purpose of this pull request?

This pull request:

  • Implements an incremental weighted mean

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

Do you agree that the weight should default to 1.0? then incrwmean with one parameter is equivalient to incrmean (though slower).

Other

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

I'm not sure that other incremental stat functions have typescript definitions. Didn't see why I shouldn't add one for this though. Let me know if there's some reason I shouldn't.

I wasn't aware of a more efficient way to implement this algorithm, I don't believe there's a shortcut like for incrmean.


@stdlib-js/reviewers

@kgryte
Copy link
Member

kgryte commented Feb 25, 2019

@MatthewCochrane This is awesome! For an incremental algorithm which only relies on maintaining the weighted mean and the sum of weights, see here.

Re: default weight. Personally, I would require a weight to always be provided. Meaning, we would not support a default weight. Otherwise, when reading code which uses incrwmean, one has to rely on "out-of-band" knowledge to know what the default weight is (a) and (b) if one is unfamiliar with the incrwmean API, it is not clear if the "missing" weight is intentional or not. Accordingly, my recommendation is that the API not be variadic.

Re: TypeScript files. Yes! We only recently started adding declaration files and have yet to add declaration files to the incr/* packages. WIP. :)

Thanks for working on this!

@MatthewCochrane
Copy link
Contributor Author

Not a problem, happy to work on it!

Yep, that's fair. I'm not sure I'd call it 'out of band knowledge' since it's in the jsdoc header, and most (sane) people use editors with documentation lookup these days. Though the saving would be quite small and you're right that it's probably worth the small amount of extra verbosity to save potential misinterpretations. I removed the default, as suggested :).

Thanks, for that link. I've updated the algorithm to use the one from the paper. It benchmarks very slightly faster than the one I had implemented.

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality. labels Feb 25, 2019
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.

@MatthewCochrane Thanks for updating! Left some comments and suggestions. Once resolved, this should be ready for merge!

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.

@MatthewCochrane Looks great! Thanks!

@kgryte kgryte merged commit 1d853ad into stdlib-js:develop Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement incrwmean

2 participants