stdlib icon indicating copy to clipboard operation
stdlib copied to clipboard

[WIP] Add Rademacher statistical distribution

Open justin1dennison opened this issue 7 years ago • 11 comments

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 develop branch.

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

justin1dennison avatar Nov 01 '18 13:11 justin1dennison

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 avatar Nov 01 '18 13:11 justin1dennison

@justin1dennison I am going to defer to the resident distributions expert, @Planeshifter.

@Planeshifter How would you parameterize the methods?

kgryte avatar Nov 01 '18 19:11 kgryte

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 avatar Nov 01 '18 21:11 Planeshifter

@Planeshifter That seems reasonable to me!

kgryte avatar Nov 01 '18 21:11 kgryte

@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.

justin1dennison avatar Nov 02 '18 12:11 justin1dennison

Yes, in cases like this where there are no implementations in the languages we usually compare ourselves to, we just include the JS benchmarks.

Planeshifter avatar Nov 02 '18 15:11 Planeshifter

@justin1dennison This is looking great thus far! Thanks for working on this!

kgryte avatar Nov 06 '18 18:11 kgryte

@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.

justin1dennison avatar Nov 08 '18 13:11 justin1dennison

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.

Planeshifter avatar Nov 08 '18 22:11 Planeshifter

@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?

justin1dennison avatar Mar 12 '19 16:03 justin1dennison

@Planeshifter What needs to be done in order to get this PR merged?

kgryte avatar Sep 21 '23 21:09 kgryte