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

Number improvements #1

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

dario-loi
Copy link
Contributor

Changes

Number Performance Improvements

After observing that Number initialized a mersenne twister and random generator on the stack on each call of the function integer, I moved them out of the function and stored them as a statically-initialized member of the Number class, this improves performance by skipping the initialization of these objects whenever a number is generated, meaning that the performance of integer is now closer to what one would get by directly using the standard library, a worthwhile improvement considering that Number is also used as a helper in other modules.

Number Type Safety

Changed the Number module to be header-only, this allows us to make the class a template that can work with any integral type. Through concepts we can constrain the templated type T to be of type std::integral, while allowing the user to generate a wider range of numbers from the same interface, such as unsigned ints, size_ts, etc... One immediate benefit of this is that in the Helper module we are now able to index the whole theoretical range of a std::vector, by generating a size_t instead of an int.

Quality-of-Life changes

Swapped the order of integer's arguments, it makes much more sense to have min come before max. The "max by default" behavior is provided by an overload that accepts a single parameter and correctly calls the two-arguments function, check Number.h to see what I mean. All tests have been changed accordingly (the order of parameters has been swapped to the sensible one) and a test has been added to check that the overload is correctly resolved.

dario-loi added 2 commits July 5, 2023 17:13
Modified the number class to be header-only, with static random generators and templated integral types bounded by concepts.
Fixed a linker error induced by the lack of initialization of Number's static members.
@cieslarmichal
Copy link
Owner

Good job, it's nice that you started writing documentation, I need to start writing it too :D
That performance improvement with numbers is really good :)
Do you have anything you would like to work on next?
I will prepare some ultimate TODO list but for now maybe generating float numbers?

@cieslarmichal cieslarmichal merged commit 0b50ca7 into cieslarmichal:main Jul 5, 2023
@dario-loi
Copy link
Contributor Author

Thank you,

it should be easy to provide an interface for Floats that enforces the same type guarantees as integers, do you think it would be worth it to expose an overload that allows one to change the distribution of the underlying random sampler (uniform, gaussian, gamma, binomial, etc...)?

I could also do that pretty easily while still preserving the easy defaults that we have now, requiring only the bounds and defaulting to the uniform distribution. Do you think the added complexity is worth it?

@cieslarmichal
Copy link
Owner

I think it's worth it to have a possibility to choose underlying random sampler, good idea.
About float generation it could be implemented in similar way with template std::floating_point and uniform_real_distribution.
We need to think about a name for float() function, because we cannot name it float - its reserved keyword. Do you have any idea how to name it? I am thinking about renaming integer to integerNumber() and naming float floatNumber(), what do you think about that?

@dario-loi
Copy link
Contributor Author

I got some work on my fork, but it is highly unstable, I called it decimal, a more mathematical term but it is short and not reserved.

As soon as it is stable enough I will add tests and PR the branch again.

@cieslarmichal
Copy link
Owner

Ok, sounds good, I created a repository discussion so we could chat there about features in progress :)

@cieslarmichal
Copy link
Owner

I created a simple TODO list in discussion:
#4
Please add your ideas for next steps if you have any,
Generally speaking - we are cloning https://fakerjs.dev/api/ functionalites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants