number.float is unable to return 1/max exactly

ST-DDT commented 1 year ago

Pre-Checks

[X] Follow our Code of Conduct.
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Make sure this is a Faker issue and not related to a combination with another package.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.
[X] I am willing to provide a PR.

Describe the bug

faker.number.float() is unable to return 1.

Minimal reproduction code

it('should be able to return 1', () => {
  // @ts-expect-error: access private member field
  const mersenne = faker._mersenne;
  mersenne.next = () => 0.9999999999999999; // based on the definition of mersenne.next()
  const actual = faker.number.float();
  expect(actual).toBe(1); // 💥 fails
});

Additional Context

Originally part of:

https://github.com/faker-js/faker/pull/2357

Environment Info

System:
    OS: Windows 10 10.0.22621
    CPU: (16) x64 Intel(R) Core(TM) i7-7820X CPU @ 3.60GHz
    Memory: 47.42 GB / 63.69 GB
  Binaries:
    Node: 18.17.1 - C:\Program Files\nodejs\node.EXE
    npm: 9.7.2 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.5.1 - C:\Program Files\nodejs\pnpm.CMD
  Browsers:
    Edge: Spartan (44.22621.2134.0), Chromium (116.0.1938.62)
    Internet Explorer: 11.0.22621.1

Which module system do you use?

[ ] CJS
[X] ESM

Used Package Manager

pnpm

dubzzz commented 1 year ago

It seems to be the usual behaviour for a random number generator generating values between 0 (included) and 1 (not included). For instance, Math.random does exactly the same.

ST-DDT commented 1 year ago

Then the problem is all our other methods such as faker.number.int that do generate the max value? What do you think?

Shinigami92 commented 1 year ago

Yeah, I came to the same conclusion as @dubzzz It is very unusual that you every need a 1.0, it is always 0.0 inclusive to 1.0 exclusive. Now as I worked a little bit with #2379 and also had a look into https://www.npmjs.com/package/pure-rand, I'm more and more of the opinion that a 1.0 inclusive is not wanted at all and if our number.float allows an inclusive max, then maybe we need to change that or find a real reason why it is that way.

I didn't head an eye on that until now, because I was always just interested in some fake data and not scientific correct accurate numbers. Just like give me any value between 0 and 1, I don't care if it ever bulls-eye 0.0, 0.5 or 1.0 exactly.

matthewmayer commented 1 year ago

Ints are fine to have inclusive max.

But floats intuitively don't.

ST-DDT commented 1 year ago

What about our faker.number.float() method with precision?

Shinigami92 commented 1 year ago

Team Decision:

Works as expected We need to enhance our documentation for that

faker-js / faker