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

Sized QuickCheck properties to avoid blowup #23

Merged
merged 3 commits into from Mar 10, 2016

Conversation

hdgarrood
Copy link
Contributor

Resolves #16. Here's a sample of what these generators produce (via Test.QuickCheck.sample):

Box {rows = 0, cols = 1, content = Blank}
Box {rows = 1, cols = 0, content = Text "\239"}
Box {rows = 4, cols = 0, content = SubBox AlignCenter2 AlignCenter1 (Box {rows = 1, cols = 2, content = SubBox AlignCenter1 AlignFirst (Box {rows = 1, cols = 0, content = Text "7"})})}
Box {rows = 3, cols = 1, content = Text ".0\189"}
Box {rows = 5, cols = 0, content = Text "HA\228ZQ"}
Box {rows = 9, cols = 7, content = Text "FV+yM"}
Box {rows = 12, cols = 4, content = SubBox AlignCenter1 AlignCenter2 (Box {rows = 3, cols = 2, content = Col [Box {rows = 0, cols = 0, content = SubBox AlignCenter1 AlignFirst (Box {rows = 1, cols = 0, content = Blank})}]})}
Box {rows = 0, cols = 7, content = Row [Box {rows = 5, cols = 5, content = Row [Box {rows = 2, cols = 0, content = Row [Box {rows = 1, cols = 0, content = SubBox AlignFirst AlignLast (Box {rows = 1, cols = 0, content = Row []})}]}]},Box {rows = 0, cols = 0, content = Blank},Box {rows = 3, cols = 1, content = Blank},Box {rows = 5, cols = 0, content = Text "+\ETX\153\ESC6"},Box {rows = 2, cols = 7, content = Col [Box {rows = 0, cols = 2, content = Row []},Box {rows = 0, cols = 1, content = Row []},Box {rows = 0, cols = 2, content = Row []}]},Box {rows = 6, cols = 4, content = Row [Box {rows = 0, cols = 3, content = SubBox AlignCenter2 AlignLast (Box {rows = 1, cols = 1, content = Row []})},Box {rows = 1, cols = 0, content = SubBox AlignCenter2 AlignLast (Box {rows = 1, cols = 0, content = Col []})}]},Box {rows = 3, cols = 6, content = Text "\150^@\238\DLE"}]}
Box {rows = 4, cols = 6, content = Text "v\SI\179\ETB"}
Box {rows = 8, cols = 4, content = Blank}
Box {rows = 0, cols = 0, content = SubBox AlignLast AlignCenter2 (Box {rows = 3, cols = 0, content = SubBox AlignFirst AlignCenter1 (Box {rows = 0, cols = 1, content = Text "m"})})}

@treeowl
Copy link
Collaborator

treeowl commented Mar 9, 2016

Thanks a lot. Could you please add comments explaining the design/choices?

@treeowl
Copy link
Collaborator

treeowl commented Mar 9, 2016

Also, please use quot instead of div. I know it's a tiny thing, but I can't stand any use of div that doesn't make specific use of its particular properties.

@hdgarrood
Copy link
Contributor Author

Sure, will do, when I get a moment. I wasn't aware of div vs quot - are the properties you're talking about things like behaviour for negative numbers? Why is that?

@treeowl
Copy link
Collaborator

treeowl commented Mar 10, 2016

Thanks. div is "Knuthian" division. It behaves better for negative operands, but

  1. It doesn't behave as well as "Euclidean" division, which guarantees a non-negative remainder.
  2. It's not only slower than quot (machine division with error handling), but also slower than Euclidean division.

So it annoys me to see it used when the numerator and denominator are known to be non-negative.

@hdgarrood
Copy link
Contributor Author

Makes sense, thanks. How's this?

@hdgarrood
Copy link
Contributor Author

Sorry, I meant to use a different account for these commits. One second.

@hdgarrood
Copy link
Contributor Author

Ok, this is ready to re-review/merge.

@treeowl
Copy link
Collaborator

treeowl commented Mar 10, 2016

Almost there! I have just one last request before I merge. The Gen Functor instance is quite different from its Monad instance, and if I have my way its Applicative instance will soon be as well. Could you replace liftM with fmap or <$> please, and liftM3 with liftA3, etc.? If not, I'll merge and fix up later; my git skills aren't really up to fixing while merging.

@hdgarrood
Copy link
Contributor Author

Cool. How's this?

@hdgarrood
Copy link
Contributor Author

Ah, I suppose we might want to import Control.Applicative for GHC <= 7.8?

@treeowl
Copy link
Collaborator

treeowl commented Mar 10, 2016

Definitely. Thanks again.
On Mar 10, 2016 3:53 PM, "Harry Garrood" notifications@github.com wrote:

Ah, I suppose we might want to import Control.Applicative for GHC <= 7.8?


Reply to this email directly or view it on GitHub
#23 (comment).

@treeowl
Copy link
Collaborator

treeowl commented Mar 10, 2016

Be sure to use MIN_VERSION_base to check--it's the polite way.
On Mar 10, 2016 4:03 PM, "David Feuer" david.feuer@gmail.com wrote:

Definitely. Thanks again.
On Mar 10, 2016 3:53 PM, "Harry Garrood" notifications@github.com wrote:

Ah, I suppose we might want to import Control.Applicative for GHC <= 7.8?


Reply to this email directly or view it on GitHub
#23 (comment).

@hdgarrood
Copy link
Contributor Author

I've tested this with GHC 7.8.4 and 7.10.3. :)

treeowl added a commit that referenced this pull request Mar 10, 2016
Sized QuickCheck properties to avoid blowup
@treeowl treeowl merged commit dd738a1 into haskell-boxes:master Mar 10, 2016
@treeowl
Copy link
Collaborator

treeowl commented Mar 10, 2016

Perfect. Merged!

@hdgarrood hdgarrood deleted the quickcheck-tests branch March 10, 2016 21:29
@treeowl
Copy link
Collaborator

treeowl commented Mar 10, 2016

Actually, one last question! Shouldn't the size be reduced by at most a fixed constant amount in the SubBox case?

@hdgarrood
Copy link
Contributor Author

I don't really know, but this seemed to work and the sample seemed reasonable :) I have a suspicion that we might still suffer from the original problem if we do that, but I can give it a try. What fixed constant amount did you have in mind?

@treeowl
Copy link
Collaborator

treeowl commented Mar 10, 2016

I don't think we'll suffer the original problem even if we don't resize
this case, because long sub-box chains will (I think) be unlikely anyway.
Or am I missing something fundamental about size? I would think cutting it
by 1 should be sufficient for consistency, no?
On Mar 10, 2016 4:34 PM, "Harry Garrood" notifications@github.com wrote:

I don't really know, but this seemed to work and the sample seemed
reasonable :) I have a suspicion that we might still suffer from the
original problem if we do that, but I can give it a try. What fixed
constant amount did you have in mind?


Reply to this email directly or view it on GitHub
#23 (comment).

@treeowl
Copy link
Collaborator

treeowl commented Mar 10, 2016

TBH, the whole size thing strikes me as rather ad hoc, a flaw in Gen.
On Mar 10, 2016 4:41 PM, "David Feuer" david.feuer@gmail.com wrote:

I don't think we'll suffer the original problem even if we don't resize
this case, because long sub-box chains will (I think) be unlikely anyway.
Or am I missing something fundamental about size? I would think cutting it
by 1 should be sufficient for consistency, no?
On Mar 10, 2016 4:34 PM, "Harry Garrood" notifications@github.com wrote:

I don't really know, but this seemed to work and the sample seemed
reasonable :) I have a suspicion that we might still suffer from the
original problem if we do that, but I can give it a try. What fixed
constant amount did you have in mind?


Reply to this email directly or view it on GitHub
#23 (comment).

@hdgarrood
Copy link
Contributor Author

Hm, yes. I'm not sure how I'd even go about designing an alternative that didn't have it, though...

It appears I was wrong; still works fine if we only decrement by 1 for SubBox. Here's a sample:

Box {rows = 1, cols = 0, content = Blank}
Box {rows = 0, cols = 0, content = Blank}
Box {rows = 4, cols = 4, content = Blank}
Box {rows = 4, cols = 0, content = Blank}
Box {rows = 1, cols = 8, content = Blank}
Box {rows = 9, cols = 0, content = Blank}
Box {rows = 5, cols = 6, content = Text "(\SI\GSe\ESC\DLEhhR\251\EM\152"}
Box {rows = 5, cols = 10, content = Row [Box {rows = 5, cols = 1, content = Col [Box {rows = 2, cols = 3, content = Col []},Box {rows = 2, cols = 3, content = Blank},Box {rows = 0, cols = 1, content = Text "\\Ke"}]},Box {rows = 0, cols = 0, content = Text "ZW\EM"},Box {rows = 6, cols = 0, content = SubBox AlignCenter2 AlignLast (Box {rows = 5, cols = 6, content = Text "\STX"})},Box {rows = 1, cols = 4, content = Col [Box {rows = 2, cols = 1, content = Col []},Box {rows = 1, cols = 2, content = Blank}]},Box {rows = 1, cols = 2, content = Col [Box {rows = 1, cols = 0, content = Text ">U}"}]}]}
Box {rows = 11, cols = 11, content = Blank}
Box {rows = 13, cols = 11, content = Col [Box {rows = 0, cols = 0, content = Col [Box {rows = 0, cols = 2, content = Col []},Box {rows = 4, cols = 0, content = Text ""},Box {rows = 2, cols = 0, content = Blank}]},Box {rows = 1, cols = 5, content = Text "B\149\175\r_\\"},Box {rows = 0, cols = 0, content = Col [Box {rows = 1, cols = 4, content = Text "q,Z"},Box {rows = 1, cols = 2, content = Row [Box {rows = 0, cols = 0, content = Col []},Box {rows = 0, cols = 0, content = Row [Box {rows = 0, cols = 0, content = Row []}]}]},Box {rows = 0, cols = 3, content = Blank},Box {rows = 4, cols = 3, content = Text "\169}"}]},Box {rows = 0, cols = 3, content = Col []},Box {rows = 4, cols = 7, content = Blank},Box {rows = 3, cols = 0, content = Col [Box {rows = 2, cols = 4, content = Text "\SYNL\GS\143"},Box {rows = 4, cols = 2, content = Text ""}]},Box {rows = 0, cols = 7, content = Blank},Box {rows = 7, cols = 4, content = Col []},Box {rows = 5, cols = 9, content = Col [Box {rows = 2, cols = 0, content = Blank},Box {rows = 2, cols = 4, content = Col []}]}]}
Box {rows = 18, cols = 0, content = Row [Box {rows = 0, cols = 1, content = Blank},Box {rows = 4, cols = 3, content = SubBox AlignCenter2 AlignLast (Box {rows = 2, cols = 1, content = Text "\f\254\254Y"})},Box {rows = 0, cols = 0, content = Text ""},Box {rows = 10, cols = 4, content = Blank},Box {rows = 5, cols = 1, content = Blank},Box {rows = 1, cols = 2, content = SubBox AlignCenter1 AlignLast (Box {rows = 9, cols = 2, content = Blank})},Box {rows = 0, cols = 3, content = Col [Box {rows = 1, cols = 2, content = Row [Box {rows = 1, cols = 2, content = Text "$"}]}]},Box {rows = 10, cols = 3, content = Text "x"}]}

@treeowl
Copy link
Collaborator

treeowl commented Mar 10, 2016

FYI, here's my intuition: a SubBox is only larger than its contents by one
constructor and two Ints, so to make a SubBox of size n, I think you want
to wrap up a box of size n-1. The same goes for Text. This is very
different from a row or column, which makes multiple smaller boxes. The
"size" notion doesn't really seem to address the question of how long a row
should be vs. how large its elements should be, which should presumably be
balanced based on intuition about whether wider or deeper tests will catch
more bugs.
On Mar 10, 2016 4:51 PM, "Harry Garrood" notifications@github.com wrote:

Hm, yes. I'm not sure how I'd even go about designing an alternative that
didn't have it, though...

It appears I was wrong; still works fine if we only decrement by 1 for
SubBox. Here's a sample:

Box {rows = 1, cols = 0, content = Blank}
Box {rows = 0, cols = 0, content = Blank}
Box {rows = 4, cols = 4, content = Blank}
Box {rows = 4, cols = 0, content = Blank}
Box {rows = 1, cols = 8, content = Blank}
Box {rows = 9, cols = 0, content = Blank}
Box {rows = 5, cols = 6, content = Text "(\SI\GSe\ESC\DLEhhR\251\EM\152"}
Box {rows = 5, cols = 10, content = Row [Box {rows = 5, cols = 1, content = Col [Box {rows = 2, cols = 3, content = Col []},Box {rows = 2, cols = 3, content = Blank},Box {rows = 0, cols = 1, content = Text "\Ke"}]},Box {rows = 0, cols = 0, content = Text "ZW\EM"},Box {rows = 6, cols = 0, content = SubBox AlignCenter2 AlignLast (Box {rows = 5, cols = 6, content = Text "\STX"})},Box {rows = 1, cols = 4, content = Col [Box {rows = 2, cols = 1, content = Col []},Box {rows = 1, cols = 2, content = Blank}]},Box {rows = 1, cols = 2, content = Col [Box {rows = 1, cols = 0, content = Text ">U}"}]}]}
Box {rows = 11, cols = 11, content = Blank}
Box {rows = 13, cols = 11, content = Col [Box {rows = 0, cols = 0, content = Col [Box {rows = 0, cols = 2, content = Col []},Box {rows = 4, cols = 0, content = Text ""},Box {rows = 2, cols = 0, content = Blank}]},Box {rows = 1, cols = 5, content = Text "B\149\175\r_"},Box {rows = 0, cols = 0, content = Col [Box {rows = 1, cols = 4, content = Text "q,Z"},Box {rows = 1, cols = 2, content = Row [Box {rows = 0, cols = 0, content = Col []},Box {rows = 0, cols = 0, content = Row [Box {rows = 0, cols = 0, content = Row []}]}]},Box {rows = 0, cols = 3, content = Blank},Box {rows = 4, cols = 3, content = Text "\169}"}]},Box {rows = 0, cols = 3, content = Col []},Box {rows = 4, cols = 7, content = Blank},Box {rows = 3, cols = 0, content = Col [Box {rows = 2, cols = 4, content = Text "\SYNL\GS\143"},Box {rows = 4, cols = 2, content = Text ""}]},Box {rows = 0, cols = 7, content = Blank},Box {rows = 7, cols = 4, content = Col []},Box {rows = 5, cols = 9, content = Col [Box {rows = 2, cols = 0,
content = Blank},Box {rows = 2, cols = 4, content = Col []}]}]}
Box {rows = 18, cols = 0, content = Row [Box {rows = 0, cols = 1, content = Blank},Box {rows = 4, cols = 3, content = SubBox AlignCenter2 AlignLast (Box {rows = 2, cols = 1, content = Text "\f\254\254Y"})},Box {rows = 0, cols = 0, content = Text ""},Box {rows = 10, cols = 4, content = Blank},Box {rows = 5, cols = 1, content = Blank},Box {rows = 1, cols = 2, content = SubBox AlignCenter1 AlignLast (Box {rows = 9, cols = 2, content = Blank})},Box {rows = 0, cols = 3, content = Col [Box {rows = 1, cols = 2, content = Row [Box {rows = 1, cols = 2, content = Text "$"}]}]},Box {rows = 10, cols = 3, content = Text "x"}]}


Reply to this email directly or view it on GitHub
#23 (comment).

@hdgarrood
Copy link
Contributor Author

Right, yeah, that makes sense. I agree.

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.

None yet

2 participants