-
Notifications
You must be signed in to change notification settings - Fork 32
[Peras #1] Introduce basic types to support Peras #1673
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
base: main
Are you sure you want to change the base?
Conversation
d3cd2fc
to
8df7902
Compare
738daaf
to
f3fbd4f
Compare
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
f3fbd4f
to
01e1c69
Compare
import Test.Tasty.Bench | ||
|
||
data BenchmarkParams = BenchmarkParams | ||
{ blockRate :: SlotNo |
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.
Aren't rates expressed as ratios? This might be a misnomer.
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.
Changed to
{ slotGap :: Word64
-- ^ The slot gap between blocks on the fragments, ie the inverse of the
-- active slot coefficient.
in 7faa0d1
{ blockRate :: SlotNo | ||
-- ^ How often the fragments will contain blocks, in slots | ||
, fragmentLenghtSamplingRate :: Natural | ||
-- ^ The rate of length increase for generate chain fragments |
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.
"generated"?
Also the comment does not seem to refer to sampling. I wonder if this field deserves a better explanation.
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.
Changed to
, fragmentLengthStepSize :: Natural
-- ^ Step size for the fragment lengths between different benchmarks.
in 7faa0d1
, fragmentLenghtSamplingRate :: Natural | ||
-- ^ The rate of length increase for generate chain fragments | ||
, fragmentMaxLenght :: Natural | ||
-- ^ the maximum length of a fragment |
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.
-- ^ the maximum length of a fragment | |
-- ^ The maximum length of a fragment |
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.
Does this Natural
express a number of slots?
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.
Does this
Natural
express a number of slots?
It is a number of blocks, added to the comment in 7faa0d1
-- implemented by the by the | ||
-- 'Ouroboros.Consensus.Peras.Weight.weightBoostOfFragment' function. | ||
-- | ||
-- We benchmark the calculation on a static sequence of chain fragments of increasing |
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.
Wouldn't it make sense to refer to benchmarkParams
directly. Otherwise we'll have to modify this comment as well if we alter said variable.
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.
Good idea, done in 7faa0d1
snap = mkPerasWeightSnapshot $ Map.toList tsWeights | ||
|
||
weightBoostOfPointReference :: Point TestBlock -> PerasWeight | ||
weightBoostOfPointReference pt = Map.findWithDefault mempty pt tsWeights |
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.
Aren't we re-implementing weightBoostOfPoint
here?
weightBoostOfPoint (PerasWeightSnapshot weightByPoint) pt =
Map.findWithDefault mempty pt weightByPoint
If so, I wonder if this test adds much value. It seems a couple of unit tests would be more useful.
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.
Yes, though the actual implementation also changes in #1613, just like you already mentioned in #1673 (comment)
So the tests are currently a bit degenerate indeed; I think it is a good idea to add some simple unit tests and a property like you suggest in #1673 (comment) 👍
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.
Actually, we already have unit tests in this PR via the doctests in the main module, which are also checked via CI since #1622.
ouroboros-consensus/test/consensus-test/Test/Consensus/Peras/WeightSnapshot.hs
Show resolved
Hide resolved
"PerasWeightSnapshot" | ||
[ testProperty "correctness" prop_perasWeightSnapshot | ||
] | ||
|
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 wonder if we could express the correctness through some algebraic properties that need not copy the implementation. Something like:
weight [] = empty
weight (x:xs) = weight x <> weight xs
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.
Added this (slightly generalized) in c4959e2
|
||
shrink ts = | ||
concat | ||
[ [ ts{tsWeights = Map.fromList tsWeights'} |
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.
Can't we shrink the map directly to avoid a fromList
/toList
conversion?
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.
AFAICT this would require an Arbitrary
instance for Point TestBlock
, and I don't really see a good way to shrink individual points without further contexts.
[ [ ts{tsWeights = Map.fromList tsWeights'} | ||
| tsWeights' <- | ||
shrinkList | ||
-- Shrink boosted points to have weight 1. |
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 assigning them weight 1 instead of shrinking the weight as well?
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.
Yeah, we could shrink the weight incrementally (reducing it until it is 1
and then remove the point).
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.
Done in 28e845e
data BenchmarkParams = BenchmarkParams | ||
{ blockRate :: SlotNo | ||
-- ^ How often the fragments will contain blocks, in slots | ||
, fragmentLenghtSamplingRate :: Natural |
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.
this should be length
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.
Fixed in 7faa0d1
import Test.Tasty.Bench | ||
|
||
data BenchmarkParams = BenchmarkParams | ||
{ blockRate :: SlotNo |
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.
separately to Damian's comment, is this using SlotNo
to represent to a period between two different slots? if so, should it be a different SlotPeriod
or SlotDifference
type?
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 a slot gap 👍 We sadly don't have a dedicated type for that, eg the HFC uses just Word64
(see eg countSlots
). I changed it to Word64
for consistency in 7faa0d1.
(Long-term, we could adopt some proper torsor stuff for this common relationship of absolute values and "distances" between them)
deriving stock Show | ||
|
||
instance Arbitrary TestSetup where | ||
arbitrary = do |
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.
could this arbitrary
be split out a bit so things have names / type annotations and it's a bit easier to follow?
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.
Good point, I added comments and factored out some stuff in ef2d685
This pull request introduces some base datatypes and functionality required for Peras, along with supporting tests, benchmarks, and documentation.
Specifically, it adds:
PerasWeight
andPerasCert
, which associate a weight with a block.PerasWeightSnapshot
, a type for capturing the known weight state of blocksPeras types and weight computation
Ouroboros.Consensus.Block.SupportsPeras
, defining types and logic for Peras weights and certificates.Ouroboros.Consensus.Block
.Ouroboros.Consensus.Peras.Weight
, introducingPerasWeightSnapshot
and weight computation functions for chains and fragments.Documentation
glossary.md
.Tests and benchmarks
PerasCertDB-bench
, including a weight computation microbenchmark (bench/PerasCertDB-bench/Main.hs
).Test.Consensus.Peras.WeightSnapshot
with property-based tests for weight computation on chains and fragments.Regression
The new code added with this PR is not actually used, so it shouldn't introduce regressions.