-
Notifications
You must be signed in to change notification settings - Fork 32
O.C.Peras.Weight: add totalWeightForFragment
/takeVolatileSuffix
#1621
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
Conversation
This is purely for concise QuickCheck counterexample output.
0ae95b6
to
5cba290
Compare
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.
Looks good to me!
Some pieces of the tests were a bit hard to understand for me; I left a few comments.
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Peras/Weight.hs
Outdated
Show resolved
Hide resolved
|
||
prop_perasWeightSnapshot :: TestSetup -> Property | ||
prop_perasWeightSnapshot testSetup = | ||
tabulate "log₂ # of points" [show $ round @Double @Int $ logBase 2 (fromIntegral (length tsPoints))] |
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.
Why do we round to the nearest integer?
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 get a reasonable amount of labelling output; if we didn't round (or floor
/ceiling
, doesn't matter), then we would get one line of output for every amount of points that is present in some test case. This way, we essentially get "buckets" of exponentially increasing size.
The high-level motivation for this is to get an idea of how many points are present in this test, as the test is rather degenerate if there are only very few (and the test would be unnecessarily slow if there were, say, millions of points).
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.
I wasn't sure if the rounding was just for displaying, or also for producing the test cases.
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.
It is indeed purely affecting "metrics" of the property tests; generation/execution of test cases is completely unaffected 👍
ouroboros-consensus/test/consensus-test/Test/Consensus/Peras/WeightSnapshot.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/Peras/WeightSnapshot.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/Peras/WeightSnapshot.hs
Show resolved
Hide resolved
5cba290
to
4c1afc8
Compare
Also add a test for
PerasWeightSnapshot
. The tests forboostedWeightOf{Point,Fragment}
are currently not that useful as the model implementation and the actual implementation are almost the same, but this will change with #1613. The test fortakeVolatileSuffix
is already useful.