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

datatype.float does not return a float with given precision #345

Closed
Shinigami92 opened this issue Jan 29, 2022 · 6 comments
Closed

datatype.float does not return a float with given precision #345

Shinigami92 opened this issue Jan 29, 2022 · 6 comments
Labels
c: bug Something isn't working m: datatype Something is referring to the datatype module

Comments

@Shinigami92
Copy link
Member

Describe the bug

The code indicates that float can take a number that will be used as precision value.
But it returns always a integer number when using typoef options === 'number'

Reproduction

faker.seed(42)
faker.datatype.float(6) === 37452

Additional Info

I found this bug while rewriting the datatype tests in #344

@ST-DDT
Copy link
Member

ST-DDT commented Feb 22, 2022

This behavior might be intentional:

faker.datatype.number(1000) // 64000
faker.datatype.float(1000) // 64000

@piotrekn
Copy link
Contributor

After further investigation, I think you're right @ST-DDT - it looks like it's a feature, not a bug. The precision option is set, but the name precision might be a little unfortunate here. It does not refer only to significant decimal digits, as this unit test exposes.

faker.datatype.number(1000) // 64000
faker.datatype.float(1000) // 64000

Are you 100% sure about the faker.datatype.number(1000) output? I think this should be random number form [0;1000]?

faker.seed(42)
faker.datatype.number(1000) // 374

faker.seed(42)
faker.datatype.float(1000) // 37000

@ST-DDT
Copy link
Member

ST-DDT commented Feb 23, 2022

Are you 100% sure about the faker.datatype.number(1000) output?

I meant faker.datatype.number({ precision: 1000 })); // 37000.
IMO precision is a good word for it (or at least I cannot think of a better one right now).

@piotrekn
Copy link
Contributor

IMO precision is a good word for it (or at least I cannot think of a better one right now).

It resembles precision if represented with 10^n, but in reality it can take any Number. What is this number in those cases, then? How would you explain what's going on? I find it hard to put it in simple words and the term "precision" is misused imho :) I also find it very weird how the precision influence the outcome:

faker.seed(42);
faker.datatype.float({ precision: 1 }   // 37454

faker.seed(42);
faker.datatype.float({ precision: 9 }   // 37449

faker.seed(42);
faker.datatype.float({ precision: 10 }  // 37450

faker.seed(42);
faker.datatype.float({ precision: 11 }  // 37455

Correct me if I'm wrong, but this has little in common to precision as typically the mathematical precision is represented as integer, and refers to the number of significant decimal digits.
I think it should work like Number.toPrecision, but in decimal notation:
image
I doubt it is expected of that Issue to create that massive breaking change right now, is it?
It's not my call, I'm happy to go back and provide the PR anytime the expected behavior is established.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 23, 2022

I doubt it is expected of that Issue to create that massive breaking change right now, is it?

Yes, you are right. We have to put more thoughts into this.

@ST-DDT ST-DDT removed this from the v6.1 - First bugfixes milestone Feb 23, 2022
@ST-DDT ST-DDT added the s: pending triage Pending Triage label Feb 23, 2022
@import-brain import-brain removed the s: pending triage Pending Triage label Mar 5, 2022
@ST-DDT ST-DDT added the s: pending triage Pending Triage label Mar 7, 2022
@Shinigami92 Shinigami92 added s: needs decision Needs team/maintainer decision and removed s: pending triage Pending Triage labels Mar 8, 2022
@xDivisionByZerox xDivisionByZerox added the m: datatype Something is referring to the datatype module label Jul 30, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Jan 26, 2023

@ST-DDT ST-DDT closed this as not planned Won't fix, can't repro, duplicate, stale Jan 26, 2023
@xDivisionByZerox xDivisionByZerox removed the s: needs decision Needs team/maintainer decision label Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working m: datatype Something is referring to the datatype module
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants