Skip to content

feat: add random/base/tinymt32 #6177

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 22 commits into
base: develop
Choose a base branch
from
Open

feat: add random/base/tinymt32 #6177

wants to merge 22 commits into from

Conversation

abdelrahman04
Copy link
Contributor

@abdelrahman04 abdelrahman04 commented Mar 19, 2025 •
edited
Loading

Resolves #200 .

Description

What is the purpose of this pull request?

This pull request:

  • Draft PR for Adding TinyMT32, a pseudo random number generator

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.

Checklist

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

@stdlib-js/reviewers

@stdlib-bot
Copy link
Contributor

Hello! Thank you for your contribution to stdlib.

We noticed that the contributing guidelines acknowledgment is missing from your pull request. Here's what you need to do:

  1. Please read our contributing guidelines.

  2. Update your pull request description to include this checked box:

    - [x] Read, understood, and followed the [contributing guidelines](https://github.com/stdlib-js/stdlib/blob/develop/CONTRIBUTING.md)

This acknowledgment confirms that you've read the guidelines, which include:

  • The developer's certificate of origin
  • Your agreement to license your contributions under the project's terms

We can't review or accept contributions without this acknowledgment.

Thank you for your understanding and cooperation. We look forward to reviewing your contribution!

@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Mar 19, 2025
@abdelrahman04
Copy link
Contributor Author

@kgryte Hello Athan,
I believe the implementation is all set
just missing the correct initialization from a seed array, it isn't done in the original author's implementation so I will check more on what to do in that case,
I made sure to have the tinymt32 match the mt19937 format in functions and descriptions mostly
for the normalized function, I followed your approach of the 2^53 precision instead of the authors way ( just multiplying by 1 / MAX Unsigned Int range).
Also, I know the linting has some issues, I just want to make sure everything is good while finalizing that and would like to know if you know of a vscode extension to point out the linting errors
Thanks

@kgryte kgryte changed the title Tiny random feat: add random/base/tinymt Mar 19, 2025
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Mar 19, 2025
@kgryte kgryte changed the title feat: add random/base/tinymt feat: add random/base/tinymt32 Mar 19, 2025
@kgryte
Copy link
Member

kgryte commented Mar 19, 2025

Yes, this PR follows the general format that we are looking for. The package needs significant clean-up, however. I suggest ensuring that you have followed the project development guide as there it tells you that you should have setup EditorConfig, etc.

For some recommended VSCode configurations, see https://github.com/stdlib-js/stdlib/tree/develop/docs/editors/vscode. It is possible, however, that that guide may be slightly out-of-date.

@abdelrahman04
Copy link
Contributor Author

@kgryte
Hello Athan,
I think all linting has been made correctly, the package is almost ready and waiting for review.
1- for the example/c/example.c I had to rewrite the tinymt32.h again since it didn't capture it outside from it's directory no matter what, I tried to look at mt19937 of handling it but found that the package is messy and doesn't even compile for the c code any more so I think it needs cleaning that can be addressed in an issue.
2- for the commits, I had to commit multiple times which were redundant too, should I create another PR with a clean branch that only has 1 commit?
Thanks for your review.

@kgryte
Copy link
Member

kgryte commented Mar 20, 2025

I tried to look at mt19937 of handling it but found that the package is messy and doesn't even compile for the c code any more so I think it needs cleaning that can be addressed in an issue.

I am not sure I follow. Here is what I see when compiling the MT19937 C example:

$ make examples-c EXAMPLES_FILTER=".*/random/base/mt19937/.*"

Running example: /stdlib/stdlib/lib/node_modules/@stdlib/random/base/mt19937/examples/c/example.c
Resolving package path...
Package path: /stdlib/stdlib/lib/node_modules/@stdlib/random/base/mt19937
Resolving package manifest...
Successfully resolved package manifest.
Resolving include directories...
Resolving source files...
Resolving libraries...
Resolving library paths...
Compiling example...
Successfully compiled example.
Success!

seed = 12345
name = mt19937
min = 0
max = 4294967295

Pseudorandom integers...
1789368711
3146859322
43676229
3522623596
3544234957
3448207591
1282648386
3672791226
1582316135
4001984784

min (normalized) = 0.0000000000000000
max (normalized) = 0.9999999999999999

Pseudorandom doubles...
0.1936613490450743
0.5660081687288613
0.1616878239293682
0.1242668842835302
0.4329362680099159
0.5620784880758429
0.1743435607237318
0.5532210855693298
0.3549013863365987
0.9580647850995486

It is quite possible that you are not compiling things correctly.

@abdelrahman04
Copy link
Contributor Author

@kgryte
Hello Athan,
I Corrected all reviewed issues, ran the following

make examples EXAMPLES_FILTER=".*/random/base/tinymt32/.*"
make examples-c EXAMPLES_FILTER=".*/random/base/tinymt32/.*"
make benchmark BENCHMARKS_FILTER=".*/random/base/tinymt32/.*"
make benchmark-c BENCHMARKS_FILTER=".*/random/base/tinymt32/.*"
make test TESTS_FILTER=".*/random/base/tinymt32/.*"

I don't know if there is anything else that need to be checked but I think the PR is ready for merging, should I do another branch with 1 clean commit?
Thanks

@abdelrahman04 abdelrahman04 requested a review from kgryte March 21, 2025 12:49
@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Mar 21, 2025
@kgryte
Copy link
Member

kgryte commented Mar 22, 2025

should I do another branch with 1 clean commit?

No, as we perform squash merging.

@abdelrahman04
Copy link
Contributor Author

abdelrahman04 commented Mar 22, 2025 •
edited
Loading

No, as we perform squash merging.

@kgryte
Is it ready for merging, or does it require further change?
Thanks

@abdelrahman04
Copy link
Contributor Author

@kgryte
Should I create a new non-draft PR?
Thanks

kgryte added 5 commits April 1, 2025 02:57
@kgryte
bench: update benchmarks to actually test factory function creation
63a948c 
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: passed
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: na
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: na
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
@kgryte
bench: fix missing normalized benchmark
01565aa 
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: passed
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: na
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: na
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
@kgryte
bench: fix description
bbf9d47 
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: passed
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: na
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: na
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
@kgryte
docs: fix value limits
f486923 
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: na
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: passed
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
@kgryte
docs: update markup
3b5b5e1 
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: passed
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: na
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: na
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgryte kgryte Awaiting requested review from kgryte

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged. Needs Review A pull request which needs code review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFC: add support for generating pseudorandom numbers using TinyMT32
3 participants
@abdelrahman04 @stdlib-bot @kgryte