-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(number): add distributor functions #3375
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
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #3375 +/- ##
==========================================
- Coverage 99.97% 99.97% -0.01%
==========================================
Files 2995 2998 +3
Lines 236324 236353 +29
Branches 941 946 +5
==========================================
+ Hits 236267 236291 +24
- Misses 57 62 +5
🚀 New features to boost your workflow:
|
|
I'm also not sure whether |
|
I don't like calling it "exponentialDistribution " because you're not returning a distribution, you are returning a single value from that distribution. So I'd call it exponentialValue or floatExponential or something similar to emphasise the thing that is returned is a float or a value. |
I'm not even sure that is the case.
Good idea. I'm not finally sure which term I would like to use but stepping away from calling it I'm currently considering one of these:
|
|
Team decision The feature will be implemented by extending the already existing methods ( We are currently unsure how to name the parameter. While distribution is okay, we are not sure if this is the mathmatical correct term. |
|
Ready for review. I changed the implementations to use a |
xDivisionByZerox
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I'm looking at this PR, the more I'm falling in love with this feature's design. ❤️
I found some small things that I was noticing, while trying to get back into this topic. @ST-DDT are you still available or can I take over the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To not couple the number tests with the implementation details of our given distributor functions, we should probably use custom, "deterministic" ones in theses test cases. Additionally, we should add separate test cases for the provided distributor functions. For example: The exponential distributor is currently not being checked for a negative "base" input.
| import { uniformDistributor } from './uniform'; | ||
|
|
||
| /** | ||
| * Creates a new function that generates power-law/exponentially distributed values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add a more accessible explanation what this means as well.
| * Creates a new function that generates power-law/exponentially distributed values. | |
| * Creates a new function that generates exponentially distributed values. | |
| * This means that values on the edges (min or max based on configuration) are more likely to be generated. |
This needs to be put in the second signature as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT this is a power law distribution and not a exponential distribution in the mathematical sense.
This means that values on one side of the of the value range are far more likely than the values on the other side of the range (depending on configuration).
Or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewmayer can you give insight here? I did not study mathematics, so I can only give perspective from a user that is clueless about these things.
Feel free to take over. |
|
What is the point of the uniformDistributor? Isn't that the same as not passing a distributor at all? |
|
Should we provide a sample normal distribution function (it wouldn't be a true normal distribution because it's bounded at the min and max). But I think it would be a fairly common use case to want something like "generate me heights in centimetres, with a mean of 170cm and standard deviation of 7cm." So I think we could provide a "recipe" for that kind of distribution even if it's not strictly normal. |
To allow for using a non-nullish distributor value. |
That's fine maybe we could just clarify that in the documentation. Otherwise a developer may set the option and wonder why "nothing happens". |
|
EDIT/fix: |
You might want to start without it first.
|
|
I am a mathematician 😀 I will have a go at something that would meet expectations. |
Implements #1862
Adds a
distributoroption to faker.number.int and float. The distributor function can be used to influence the distribution of the generated values. E.g. uniformDistributor and exponentialDistributor.The following table shows the rough distribution of values generated using
exponentialDistributor({ base: x }):The exponential distribution is achieved by using
base ** exponentwhere exponent is a random float between 0 and 1.The result is then stretched evenly to fit to
minandmax.