Skip to content

Tuple instances random#72

Merged
lehins merged 3 commits intomasterfrom
tuple-instances-random
Sep 19, 2021
Merged

Tuple instances random#72
lehins merged 3 commits intomasterfrom
tuple-instances-random

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
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.

@curiousleo curiousleo mentioned this pull request Jun 30, 2020
Closed
@Shimuuar
Copy link
Contributor

Shimuuar commented Jun 30, 2020

UniformRange, which already has tuple instances.

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

@curiousleo
Copy link
Contributor

curiousleo commented Jun 30, 2020

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

Bodigrim commented Jun 17, 2021

@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

@curiousleo curiousleo curiousleo left review comments

@Bodigrim Bodigrim Bodigrim 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