Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add Rademacher statistical distribution #245

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from

Conversation

justin1dennison
Copy link
Contributor

Resolves #182 .

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:

  • Adds the Rademacher statistical distribution.

Related Issues

Does this pull request have any 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.

No.


@stdlib-js/reviewers

@justin1dennison
Copy link
Contributor Author

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.

@kgryte
Copy link
Member

kgryte commented Nov 1, 2018

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

@Planeshifter How would you parameterize the methods?

@Planeshifter
Copy link
Member

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.

@kgryte
Copy link
Member

kgryte commented Nov 1, 2018

@Planeshifter That seems reasonable to me!

@justin1dennison
Copy link
Contributor Author

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

@Planeshifter
Copy link
Member

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

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality. Statistics Issue or pull request related to statistical functionality. labels Nov 2, 2018
@kgryte
Copy link
Member

kgryte commented Nov 6, 2018

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

@justin1dennison
Copy link
Contributor Author

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

@Planeshifter
Copy link
Member

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.


<!-- <equation class="equation" label="eq:rademacher_median" align="center" raw="\operatorname{Median}\left( X \right) = 0" alt="Median for a Rademacher distribution."> -->

<div class="equation" align="center" data-raw-text="\operatorname{Median}\left( X \right) = 0" data-equation="eq:bernoulli_median">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div class="equation" align="center" data-raw-text="\operatorname{Median}\left( X \right) = 0" data-equation="eq:bernoulli_median">
<div class="equation" align="center" data-raw-text="\operatorname{Median}\left( X \right) = 0" data-equation="eq:rademacher_median">

Returns the [median][median] of a [Rademacher][rademacher-distribution] distribution.

```javascript
var v = median( );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var v = median( );
var v = median();

var median = require( '@stdlib/stats/base/dists/rademacher/median' );
```

#### median( )
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#### median( )
#### median()


<section class="links">

[rademacher-distribution]: https://en.wikipedia.org/wiki/Bernoulli_distribution
Copy link
Member

Choose a reason for hiding this comment

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

This URL will need updating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sure will. 😄

b.tic();
for ( i = 0; i < b.iterations; i++ ) {
// NOTE: certain compiler optimizations may render this benchmark meaningless...
y = median( );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
y = median( );
y = median();

@justin1dennison justin1dennison force-pushed the rfc-182-rademacher-dist branch from 1283872 to b7a0a28 Compare March 21, 2019 12:03
@kgryte kgryte removed the Math Issue or pull request specific to math functionality. label Jun 26, 2020
@kgryte
Copy link
Member

kgryte commented Sep 21, 2023

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

@kgryte kgryte added Needs Review A pull request which needs code review. status: Blocked Issue or pull request which is current blocked. labels Feb 23, 2024
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. Needs Review A pull request which needs code review. Statistics Issue or pull request related to statistical functionality. status: Blocked Issue or pull request which is current blocked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: add the Rademacher distribution
3 participants