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

[BUG]: setting seed=0 leads to non-deterministic behavior #2952

Closed
mglezer opened this issue Sep 26, 2024 · 3 comments · Fixed by #3007
Closed

[BUG]: setting seed=0 leads to non-deterministic behavior #2952

mglezer opened this issue Sep 26, 2024 · 3 comments · Fixed by #3007
Labels
Bug Something isn't working. priority: High High priority concern or feature request.

Comments

@mglezer
Copy link

mglezer commented Sep 26, 2024

Description

I expect that setting the seed to any positive integer, including zero, should lead to deterministic behavior. This is, for example, how Python's built-in random library works. However this is not the case for @stdlib/random-shuffle; the seed seems to be silently ignored. Either seeding with zero should produce deterministic behavior, or it should fail loudly, just like with negative seed values.

Related Issues

Related issues # , # , and # .

Questions

No.

Demo

No response

Reproduction

In the node REPL:


> const shuffle = require('@stdlib/random-shuffle')
> shuffle.factory({ seed: 0 })([1,2,3])
> shuffle.factory({ seed: 0 })([1,2,3])


### Expected Results

```shell
Either

> const shuffle = require('@stdlib/random-shuffle')
> shuffle.factory({ seed: 0 })([1,2,3])
[[ SOME PERMUTATION ]]
> shuffle.factory({ seed: 0 })([1,2,3])
[[ THE SAME PERMUTATION ]]

or, throw an error and require strictly positive seeds.



### Actual Results

```shell
> const shuffle = require('@stdlib/random-shuffle')
> shuffle.factory({ seed: 0 })([1,2,3])
[ 3, 2, 1 ]
> shuffle.factory({ seed: 0 })([1,2,3])
[ 2, 3, 1 ]


### Version

0.2.1

### Environments

Node.js

### Browser Version

_No response_

### Node.js / npm Version

_No response_

### Platform

_No response_

### Checklist

- [X] Read and understood the [Code of Conduct](https://github.com/stdlib-js/stdlib/blob/develop/CODE_OF_CONDUCT.md).
- [X] Searched for existing issues and pull requests.
@stdlib-bot
Copy link
Contributor

👋 Hi there! 👋

And thank you for opening your first issue! We will get back to you shortly. 🏃 💨

@kgryte
Copy link
Member

kgryte commented Sep 26, 2024

@mglezer You're right. It seems to be due to the following line:

.

@Planeshifter Looking back through the code history, is there a reason why you wrote this to have a weak check (e.g., if ( config.seed ) {...})?

@kgryte kgryte added the Bug Something isn't working. label Sep 26, 2024
rishav2404 added a commit to rishav2404/stdlib that referenced this issue Oct 14, 2024
The previous implementation used a direct property check (`config.seed`),
which could inadvertently treat falsy values like `0` as missing, leading
to incorrect random seed initialization.

Changed the condition to explicitly check for the presence of the `seed`
property using `Object.prototype.hasOwnProperty.call()`. This update
ensures that seed initialization handles falsy but valid seed values
correctly.

Resolves stdlib-js#2952.

This pull request addresses the bug where setting `seed=0` leads to
non-deterministic behavior.

No related issues.

No questions for reviewers.

No additional information relevant to this pull request.

- [x] Read, understood, and followed the [contributing guidelines][contributing].

---

@stdlib-js/reviewers

[contributing]: https://github.com/stdlib-js/stdlib/blob/develop/CONTRIBUTING.md
@kgryte kgryte changed the title Setting seed=0 leads to non-deterministic behavior [BUG]: setting seed=0 leads to non-deterministic behavior Oct 18, 2024
@kgryte kgryte added the priority: High High priority concern or feature request. label Oct 18, 2024
@kgryte kgryte closed this as completed in 93560b9 Oct 19, 2024
@kgryte
Copy link
Member

kgryte commented Oct 19, 2024

@mglezer Thanks for reporting. This issue was resolved in #3007, and the patch should be made available in the next round of package releases.

cc @Planeshifter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working. priority: High High priority concern or feature request.
Projects
None yet
3 participants