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

Fix apparent performance calculation #1006

Merged
merged 8 commits into from
Nov 12, 2019
Merged

Fix apparent performance calculation #1006

merged 8 commits into from
Nov 12, 2019

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Nov 11, 2019 •
edited
Loading

Issue Number

Should probably be a bug.

Overview

  • I fixed the way we calculate S (number of slots in the epoch).

Comments

Comparison

Setup:

jormungandr --genesis-block-hash 6a40cf890d84981353457fcab6c892af57ee3c3286b33b530cd46b1af5b0e3a7 \
    --rest-listen 127.0.0.1:8080 \
    --storage /Users/Johannes/.local/share/cardano-wallet/jormungandr/testnet/chain \
    --config ../testnet.yml

cardano-wallet-jormungandr serve --genesis-block-hash 6a40cf890d84981353457fcab6c892af57ee3c3286b33b530cd46b1af5b0e3a7 --node-port 8080

Previously (uncapped)

Using the first commit to show the uncapped apparent performances.

cardano-wallet-jormungandr stake-pool list | jq '.[] | "\(.metrics.controlled_stake.quantity),\(.metrics.produced_blocks.quantity) \(.apparent_performance)"' | tr -d '"'
Ok.
10000000000000,2 2.2908997271066665
10000000000000,2 2.2908997271066665
10000000000000,1 1.1454498635533332
10000000000000,1 1.1454498635533332
799999987900,0 0
10000000000000,0 0
8039991840700,0 0
9886999984600,0 0

Now (uncapped)

cardano-wallet-jormungandr stake-pool list | jq '.[] | "\(.metrics.controlled_stake.quantity),\(.metrics.produced_blocks.quantity) \(.apparent_performance)"' | tr -d '"'
Ok.
10000000000000,2 0.19636283375199998
10000000000000,2 0.19636283375199998
10000000000000,1 0.09818141687599999
10000000000000,1 0.09818141687599999
799999987900,0 0
10000000000000,0 0
8039991840700,0 0
9886999984600,0 0

@Anviking Anviking self-assigned this Nov 11, 2019
@Anviking Anviking requested a review from KtorZ November 11, 2019 20:02
KtorZ
KtorZ reviewed Nov 11, 2019
View reviewed changes
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

Good catch 👍
Thanks for checking this!

I insist however on the necessity of keeping the value clamped to (0;1).

lib/core/src/Cardano/Pool/Metrics.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Pool/Metrics.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Pool/Metrics.hs Show resolved Hide resolved
@KtorZ
Copy link
Member

KtorZ commented Nov 12, 2019

Also, @Anviking , pointing back to the 3rd point of our QA section in the development process: https://github.com/input-output-hk/cardano-wallet/wiki/Development-Process#qa

May you add an actual regression test that illustrate the issue? This can be achieved using the calculatePerformance function only I believe, on manually-crafted data and showing what we expect. This should be added and seen failed before you introduce your fix 👍

@Anviking Anviking force-pushed the anviking/fix-performance-calculation branch from 00f2941 to d831c13 Compare November 12, 2019 14:17
KtorZ
KtorZ reviewed Nov 12, 2019
View reviewed changes
lib/core/test/unit/Cardano/Pool/MetricsSpec.hs Outdated
n <- choose (0, 100)
Lovelace <$> frequency
[ (50, return n)
, (25, return $ minLovelace - n)
Copy link
Member

Choose a reason for hiding this comment

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

What is this supposed to mean 🤔 ? I believe that for Word64, this will simply loop back to maxBound. What's the intent behind this?

Copy link
Member Author

@Anviking Anviking Nov 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, oops, that should have been maxBound - n.

But I will remove this as I needed it to avoid overflow the test that is now not here… Actually maybe it is still needed 🤔

@Anviking
Copy link
Member Author

Failing regression tests without the fix:

  calculatePerformances
    performances are always between 0 and 1
      +++ OK, passed 100 tests (12% all null).
    golden test cases
      50% stake, producing 4/8 blocks => performance=1
      50% stake, producing 8/8 blocks => performance=1
      50% stake, producing 2/8 blocks => performance=0.5 FAILED [1]
      50% stake, producing 0/8 blocks => performance=0
Cardano.Wallet.Api.Types
  can perform roundtrip JSON serialization & deserialization, and match existing golden files
    JSON encoding of ApiStakePoolMetrics
      allows to encode values with aeson and read them back
        +++ OK, passed 100 tests.
    JSON encoding of ApiStakePoolMetrics
      produces the same JSON as is found in test/data/Cardano/Wallet/Api/ApiStakePoolMetrics.json

Failures:

  test/unit/Cardano/Pool/MetricsSpec.hs:147:13:
  1) Cardano.Pool.Metrics.calculatePerformances, golden test cases, 50% stake, producing 2/8 blocks => performance=0.5
       expected: Just 0.5
        but got: Just 1.0

  To rerun use: --match "/Cardano.Pool.Metrics/calculatePerformances/golden test cases/50% stake, producing 2/8 blocks => performance=0.5/"

KtorZ
KtorZ approved these changes Nov 12, 2019
View reviewed changes
lib/core/test/unit/Cardano/Pool/MetricsSpec.hs
let stake = mkStake [ (poolA, 1), (poolB, 1) ]
let production = mkProduction [ (poolA, 2), (poolB, 0) ]
let performances = calculatePerformance 8 stake production
Map.lookup poolA performances `shouldBe` (Just 0.5)
Copy link
Member

Choose a reason for hiding this comment

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

😊

Anviking and others added 8 commits November 12, 2019 18:55
@Anviking @KtorZ
Add "goldens" for calclatePerformance
b8bc189
@Anviking @KtorZ
Fix incorrect apparent_performance calculation
655abe7 
For the parameter
>  S - total number of slots in the epoch
we used to sum the total block productions in our `Map PoolId nOfBlocks` map.

This is wrong as empty slots are not represented in the map of block
productions. Summing the map gives us the number of /blocks/ in the
epoch, not number of /slots/.
@Anviking @KtorZ
fixup remove derivingvia
f92e353
@Anviking @KtorZ
Make it a property test
a617995
@Anviking @KtorZ
Re-add goldens
8fe057a
@Anviking @KtorZ
Re-formating + extra test case
c84701b
@Anviking @KtorZ
Re-reformat & remove superflous golden
bc25a37
@KtorZ
add one extra golden test case when a pool produces more than expected
8765169
@KtorZ KtorZ force-pushed the anviking/fix-performance-calculation branch from 91bf55c to 8765169 Compare November 12, 2019 17:56
@KtorZ
Copy link
Member

KtorZ commented Nov 12, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 12, 2019
@iohk-bors @Anviking @KtorZ
Merge #1006
b64792f 
1006: Fix apparent performance calculation r=KtorZ a=Anviking

# Issue Number

Should probably be a bug.

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I fixed the way we calculate `S (number of slots in the epoch)`. 


# Comments

## Comparison

Setup:
```bash
jormungandr --genesis-block-hash 6a40cf890d84981353457fcab6c892af57ee3c3286b33b530cd46b1af5b0e3a7 \
    --rest-listen 127.0.0.1:8080 \
    --storage /Users/Johannes/.local/share/cardano-wallet/jormungandr/testnet/chain \
    --config ../testnet.yml

cardano-wallet-jormungandr serve --genesis-block-hash 6a40cf890d84981353457fcab6c892af57ee3c3286b33b530cd46b1af5b0e3a7 --node-port 8080
```

### Previously (uncapped)

Using the first commit to show the uncapped apparent performances.

```bash
cardano-wallet-jormungandr stake-pool list | jq '.[] | "\(.metrics.controlled_stake.quantity),\(.metrics.produced_blocks.quantity) \(.apparent_performance)"' | tr -d '"'
Ok.
10000000000000,2 2.2908997271066665
10000000000000,2 2.2908997271066665
10000000000000,1 1.1454498635533332
10000000000000,1 1.1454498635533332
799999987900,0 0
10000000000000,0 0
8039991840700,0 0
9886999984600,0 0
```

### Now (uncapped)

```bash
cardano-wallet-jormungandr stake-pool list | jq '.[] | "\(.metrics.controlled_stake.quantity),\(.metrics.produced_blocks.quantity) \(.apparent_performance)"' | tr -d '"'
Ok.
10000000000000,2 0.19636283375199998
10000000000000,2 0.19636283375199998
10000000000000,1 0.09818141687599999
10000000000000,1 0.09818141687599999
799999987900,0 0
10000000000000,0 0
8039991840700,0 0
9886999984600,0 0
```



<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 12, 2019

Build succeeded

@iohk-bors iohk-bors bot merged commit 8765169 into master Nov 12, 2019
@KtorZ KtorZ deleted the anviking/fix-performance-calculation branch November 12, 2019 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ approved these changes

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Anviking @KtorZ