stdlib
stdlib copied to clipboard
[WIP] Add Rademacher statistical distribution
Resolves #182 .
Checklist
Please ensure the following tasks are completed before submitting this pull request.
- [x] Read, understood, and followed the contributing guidelines, including the relevant style guides.
- [x] Read and understand the Code of Conduct.
- [x] Read and understood the licensing terms.
- [x] Searched for existing issues and pull requests before submitting this pull request.
- [x] Filed an issue (or an issue already existed) prior to submitting this pull request.
- [x] Rebased onto latest
develop. - [x] Submitted against
developbranch.
Description
What is the purpose of this pull request?
This pull request:
- Adds the Rademacher statistical distribution.
Related Issues
Does this pull request have any related issues?
This pull request:
- resolves #182
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.
No.
@stdlib-js/reviewers
I noticed that the Rademacher distribution does take two parameters like many of the other distributions that are currently in stdlib. Should there be a pmf.factory() function if that is the case? Please correct me if I am looking at this wrong.
@justin1dennison I am going to defer to the resident distributions expert, @Planeshifter.
@Planeshifter How would you parameterize the methods?
For a distribution without distribution parameters, it doesn't really make sense to offer a factory method, so it's a valid question. Let's not offer a factory method for any of the distribution functions.
@Planeshifter That seems reasonable to me!
@kgryte and @Planeshifter thanks for the help. If there isn't an existing implementation in Python, R, or the sort to benchmark, then is the one benchmark the only one required? Or maybe I am just not finding the Rademacher distribution. Thanks again.
Yes, in cases like this where there are no implementations in the languages we usually compare ourselves to, we just include the JS benchmarks.
@justin1dennison This is looking great thus far! Thanks for working on this!
@kgryte @Planeshifter I noticed that the mean ( as well as median, skewness, etc) for the Rademacher distribution are constant values. I worked on the mean with the assumption that these should still be implemented as functions to maintain the interface like the other distributions. Is that the correct assumption? Additionally, what does that mean for the tests, benchmarks, etc? Thanks.
Yes, for consistency reasons the distribution properties should still be exposed as functions. Testing would just entail that the package exports a function and that it's return value matches the intended value, with benchmarks being similarly trivial.
@kgryte Apologies for falling off the grid on this one. I have been (and still am) buried in a work project. I am good with finishing and merging this pull request and thus allowing the remaining features be implemented separately. How should I proceed?
@Planeshifter What needs to be done in order to get this PR merged?