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.

Sign up for GitHub

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

Jump to bottom

Tuple instances random #72

Merged
merged 3 commits into from
Sep 19, 2021
Merged

Tuple instances random #72

merged 3 commits into from
Sep 19, 2021

Conversation

lehins
Copy link
Contributor

@lehins lehins commented Jun 29, 2020

Fix for: #26

Instances for tuples. Improved coverage. Fixes to Random haddock.

@lehins lehins mentioned this pull request Jun 29, 2020
Closed
Bodigrim
Bodigrim reviewed Jun 29, 2020
View reviewed changes
src/System/Random.hs Show resolved Hide resolved
src/System/Random.hs Outdated Show resolved Hide resolved
curiousleo
curiousleo reviewed Jun 30, 2020
View reviewed changes
Copy link
Contributor

@curiousleo curiousleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should add features related to Random. To use these, people need to change their code anyway. I think it would be better for them to switch to using UniformRange, which already has tuple instances.

This feels like encouraging users to adopt / keep using Random.

src/System/Random.hs Outdated Show resolved Hide resolved
src/System/Random.hs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@curiousleo curiousleo mentioned this pull request Jun 30, 2020
Closed
@Shimuuar
Copy link
Contributor

UniformRange, which already has tuple instances.

UniformRange doesn't have tuple instances. And couldn't possibly have

@curiousleo
Copy link
Contributor

UniformRange, which already has tuple instances.

UniformRange doesn't have tuple instances. And couldn't possibly have

You are right of course. Uniform has tuple instances, UniformRange does not have tuple instances.

@lehins
Copy link
Contributor Author

lehins commented Jun 30, 2020

I think it would be better for them to switch to using UniformRange, which already has tuple instances.

@curiousleo This has already been discussed in #26 and it was also pointed out here that UnifromRange can't have an instance for tuples

I don't think we should add features related to Random.

We've discussed this on multiple occasions. Random is not going away and there is nothing wrong if people want to continue using it. There are people from different backgrounds using Haskell, math gurus that care about laws and others who could care less about math and they just want to get some random values. As long as it is documented, there is nothing wrong with having sensible and useful instances that don't produce uniform values on the whole range or don't obey the ordering defined for them, because it is not always possible to define some combination of Uniform or UniformRange, here are some latest examples: tuples #26, Complex numbers #74, Rational #75 and Natural #73
CC @idontgetoutmuch

@Bodigrim
Copy link
Contributor

@lehins since now we have instances for tuples both in Uniform and UniformRange, do you wish to pursue this PR further?

@lehins
Copy link
Contributor Author

lehins commented Jun 18, 2021

I'll extract some tests from this PR and will use it to compare performance of derived vs manual instances for tuples. I'll keep it open just for the time being so I don't forget to do it.

@lehins lehins mentioned this pull request Sep 11, 2021
Closed
@lehins lehins force-pushed the tuple-instances-random branch from 758afba to 01ee33f Compare September 11, 2021 01:25
@lehins
Copy link
Contributor Author

lehins commented Sep 11, 2021

@lehins since now we have instances for tuples both in Uniform and UniformRange, do you wish to pursue this PR further?

@Bodigrim After thinking for a while, yes, I believe it is a good idea to have tuples instances for Random as well.

@lehins lehins force-pushed the tuple-instances-random branch 3 times, most recently from 5caac2c to a30b4c9 Compare September 18, 2021 15:51
@lehins
Add tuple instances for Random and tests
7109b61
@lehins
Fix haddock for Random:
5065bb4 
* Fix range for floating point from `[0, 1)` to `[0, 1]`
* Remove promise of uniform values for all types
* Add notes to individual instances with their peculiarities
@lehins
Add a few doctest examples for randomR. Disable deps for unrunnable…
a850570 
… doctests
@lehins lehins force-pushed the tuple-instances-random branch from a30b4c9 to a850570 Compare September 19, 2021 17:51
@lehins lehins merged commit 0be123b into master Sep 19, 2021
@lehins lehins deleted the tuple-instances-random branch September 19, 2021 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bodigrim Bodigrim Bodigrim left review comments

@curiousleo curiousleo curiousleo left review comments

@Shimuuar Shimuuar Awaiting requested review from Shimuuar

@idontgetoutmuch idontgetoutmuch Awaiting requested review from idontgetoutmuch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lehins @Shimuuar @curiousleo @Bodigrim